Add localeconv function to locale module#4558
Conversation
youknowone
left a comment
There was a problem hiding this comment.
Thanks! I left a few comments about styles.
c73148b to
09c750c
Compare
| #[pyattr] | ||
| use libc::{ | ||
| ABDAY_1, ABDAY_2, ABDAY_3, ABDAY_4, ABDAY_5, ABDAY_6, ABDAY_7, ABMON_1, ABMON_10, ABMON_11, | ||
| ABMON_12, ABMON_2, ABMON_3, ABMON_4, ABMON_5, ABMON_6, ABMON_7, ABMON_8, ABMON_9, | ||
| ALT_DIGITS, AM_STR, CODESET, CRNCYSTR, DAY_1, DAY_2, DAY_3, DAY_4, DAY_5, DAY_6, DAY_7, | ||
| D_FMT, D_T_FMT, ERA, ERA_D_FMT, ERA_D_T_FMT, ERA_T_FMT, LC_ALL, LC_COLLATE, LC_CTYPE, | ||
| LC_MESSAGES, LC_MONETARY, LC_NUMERIC, LC_TIME, MON_1, MON_10, MON_11, MON_12, MON_2, MON_3, | ||
| MON_4, MON_5, MON_6, MON_7, MON_8, MON_9, NOEXPR, PM_STR, RADIXCHAR, THOUSEP, T_FMT, | ||
| T_FMT_AMPM, YESEXPR, | ||
| }; |
There was a problem hiding this comment.
Not all of these constants are available on every platform. When I encountered this sort of problem in the past, I looked up each item individually in the libc repository to see which platforms support that item. (See #3840, for example.)
I realized back then (and I still maintain) that this process would prove to be tedious and error-prone. Perhaps I should look into using the autocfg crate instead, but that's ultimately beyond the scope of this PR.
There was a problem hiding this comment.
I tried with Window locale.h this afternoon and found out that Window API uses different naming convention (prefix with W, if I remember). Hence, if we would like to fix it, I'm afraid that we need to implement our own locale interaction. I will do it in another PR once this module takes shape.
Right now, I put this module limit to unix system only!
…localeconv-function
|
|
|
The windows failure is fine. But test_type failure on linux is... what's happened |
|
https://github.com/RustPython/RustPython/actions/runs/4271112901/jobs/7435359720#step:12:12102 |
|
@youknowone it turns out the locale code is inconsistent between platform, so after some fixes, it's normal now! |
77af913 to
26f04e2
Compare
stdlib/src/locale.rs
Outdated
| let result = match args.locale.flatten() { | ||
| None => libc::setlocale(args.category, ptr::null()), | ||
| Some(l) => { | ||
| let l_str = CString::new(l.to_string()).expect("expect to be always converted"); |
There was a problem hiding this comment.
this is not always successful
>>> _locale.setlocale(0, '\0')
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
ValueError: embedded null character
stdlib/src/locale.rs
Outdated
| None => libc::setlocale(args.category, ptr::null()), | ||
| Some(l) => { | ||
| let l_str = CString::new(l.to_string()).expect("expect to be always converted"); | ||
| let l_ptr = CStr::as_ptr(&l_str); |
There was a problem hiding this comment.
because CString implements std::ops::Deref<Target=CStr>, this is redundant.
libc::setlocale(args.category, lstr.as_ptr())
works exactly same way.
This PR is to add localeconv function to locale module.
#3850