8000 Add localeconv function to locale module by minhrongcon2000 · Pull Request #4558 · RustPython/RustPython · GitHub
[go: up one dir, main page]

Skip to content

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

Merged

Conversation

minhrongcon2000
Copy link
Contributor
@minhrongcon2000 minhrongcon2000 commented Feb 24, 2023

This PR is to add localeconv function to locale module.

#3850

Copy link
Member
@youknowone youknowone left a 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.

@minhrongcon2000 minhrongcon2000 force-pushed the fix/add-localeconv-function branch from c73148b to 09c750c Compare February 24, 2023 14:42
@minhrongcon2000 minhrongcon2000 requested review from fanninpm and youknowone and removed request for fanninpm February 24, 2023 16:41
Comment on lines +12 to +21
#[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,
};
Copy link
Contributor

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.

Copy link
Contributor Author
@minhrongcon2000 minhrongcon2000 Feb 24, 2023

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!

@youknowone
Copy link
Member

test_setlocale_category is also successful!

@youknowone
Copy link
Member

The windows failure is fine. But test_type failure on linux is... what's happened

@minhrongcon2000
Copy link
Contributor Author

https://github.com/RustPython/RustPython/actions/runs/4271112901/jobs/7435359720#step:12:12102
@youknowone It seems like the format() function is not implemented with locale, so it returns an unformatted string, resulting in failed test case...

@youknowone youknowone added the z-ls-2023 Tag to track Line OSS Sprint 2023 label Feb 26, 2023 < B429 /div>
@minhrongcon2000
Copy link
Contributor Author

@youknowone it turns out the locale code is inconsistent between platform, so after some fixes, it's normal now!

@minhrongcon2000 minhrongcon2000 force-pushed the fix/add-localeconv-function branch from 77af913 to 26f04e2 Compare February 28, 2023 05:53
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");
Copy link
Member

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

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);
Copy link
Member

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.

Copy link
Member
@youknowone youknowone left a 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.

@youknowone youknowone merged commit af1d46f into RustPython:main Feb 28, 2023
@youknowone youknowone mentioned this pull request Mar 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
z-ls-2023 Tag to track Line OSS Sprint 2023
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants
0