Skip to content

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
@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