FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Add localeconv function to locale module by minhrongcon2000 · Pull Request #4558 · RustPython/RustPython · GitHub

Add localeconv function to locale module - #4558

Merged
youknowone merged 30 commits into
RustPython:mainfrom
minhrongcon2000:fix/add-localeconv-function
Feb 28, 2023
Merged

Add localeconv function to locale module#4558
youknowone merged 30 commits into
RustPython:mainfrom
minhrongcon2000:fix/add-localeconv-function

Conversation

minhrongcon2000 commented Feb 24, 2023
edited by youknowone
Loading

Copy link
Copy Markdown
Contributor

This PR is to add localeconv function to locale module.

#3850

youknowone left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thanks! I left a few comments about styles.

Comment thread stdlib/src/lib.rs Outdated
Comment thread stdlib/src/locale.rs Outdated
Comment thread stdlib/src/locale.rs Outdated
Comment thread stdlib/src/locale.rs Outdated
Comment thread stdlib/src/locale.rs Outdated
Comment thread stdlib/src/locale.rs Outdated
Comment thread stdlib/src/locale.rs Outdated
minhrongcon2000 force-pushed the fix/add-localeconv-function branch from c73148b to 09c750c Compare February 24, 2023 14:42
Comment thread stdlib/src/locale.rs Outdated
minhrongcon2000 requested review from fanninpm and youknowone and removed request for fanninpm February 24, 2023 16:41
Comment thread Lib/test/test_locale.py Outdated
Comment thread stdlib/src/locale.rs
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
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

minhrongcon2000 Feb 24, 2023
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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!

Comment thread stdlib/src/lib.rs
Comment thread stdlib/src/locale.rs Outdated
Comment thread stdlib/src/locale.rs Outdated
Comment thread stdlib/src/locale.rs Outdated
Comment thread stdlib/src/locale.rs Outdated
Comment thread stdlib/src/locale.rs Outdated

Copy link
Copy Markdown
Member

test_setlocale_category is also successful!

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
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 added the z-ls-2023 Tag to track Line OSS Sprint 2023 label Feb 26, 2023

Copy link
Copy Markdown
Contributor Author

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

Comment thread Lib/test/test_locale.py Outdated
Comment thread Lib/test/test_types.py Outdated
minhrongcon2000 force-pushed the fix/add-localeconv-function branch from 77af913 to 26f04e2 Compare February 28, 2023 05:53
Comment thread Lib/test/test_locale.py Outdated
Comment thread Lib/test/test_locale.py
Comment thread Lib/test/test_locale.py
Comment thread Lib/test/test_locale.py
Comment thread Lib/test/test_locale.py Outdated
Comment thread Lib/test/test_locale.py
Comment thread Lib/test/test_types.py Outdated
Comment thread Lib/test/test_locale.py Outdated
Comment thread Lib/test/test_locale.py Outdated
Comment thread Lib/test/test_locale.py Outdated
Comment thread Lib/test/test_locale.py Outdated
Comment thread 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");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

this is not always successful

>>> _locale.setlocale(0, '\0')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: embedded null character

Comment thread 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);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

because CString implements std::ops::Deref<Target=CStr>, this is redundant.

libc::setlocale(args.category, lstr.as_ptr())

works exactly same way.

youknowone left a comment
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 merged commit af1d46f into RustPython:main Feb 28, 2023
youknowone mentioned this pull request Mar 30, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
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


Back | FazBrowse Home | New Git URL