-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Add localeconv function to locale module #4558
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add localeconv function to locale module #4558
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I left a few comments about styles.
Sorry, something went wrong.
All reactions
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Sorry, something went wrong.
All reactions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
Sorry, something went wrong.
All reactions
-
👍 1 reaction
…localeconv-function
8000
|
All reactions
Sorry, something went wrong.
The windows failure is fine. But test_type failure on linux is... what's happened |
All reactions
Sorry, something went wrong.
https://github.com/RustPython/RustPython/actions/runs/4271112901/jobs/7435359720#step:12:12102 |
All reactions
Sorry, something went wrong.
@youknowone it turns out the locale code is inconsistent between platform, so after some fixes, it's normal now! |
All reactions
Sorry, something went wrong.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is not always successful
>>> _locale.setlocale(0, '\0')
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
ValueError: embedded null character
Sorry, something went wrong.
All reactions
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because CString implements std::ops::Deref<Target=CStr>
, this is redundant.
libc::setlocale(args.category, lstr.as_ptr())
works exactly same way.
Sorry, something went wrong.
All reactions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for hard working for this tricky issue. It will be a great base of the future locale works.
Maintaining note: Though a few tests regressed, I think this is the way to go forward with true locale support.
squash is desired when merging.
Sorry, something went wrong.
All reactions
fanninpm
youknowone
Successfully merging this pull request may close these issues.
This PR is to add localeconv function to locale module.
#3850