GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Add localeconv function to locale module by minhrongcon2000 · Pull Request #4558 · RustPython/RustPython

Conversation

@minhrongcon2000

@minhrongcon2000 minhrongcon2000 commented Feb 24, 2023 •
edited by youknowone
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

This PR is to add localeconv function to locale module.

#3850

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
minhrongcon2000 added 8 commits February 24, 2023 09:46
@minhrongcon2000
Add localeconv function to locale module
b3e39a2
@minhrongcon2000
Fix potential infinite loop
1ed1b29
@minhrongcon2000
Skip locale test
7e9390c
@minhrongcon2000
Refactor and add platform def for localeconv
b2c1e5f
@minhrongcon2000
Add missing platform def on constant
0dd95e0
@minhrongcon2000
Fix wrong format
65f5036
@minhrongcon2000
Fix platform dependent error build
359c696
@minhrongcon2000
Add platform def at the top of locale module
1533f7b
youknowone
youknowone requested changes Feb 24, 2023
View reviewed changes

@youknowone youknowone left a comment

Copy link
Copy Markdown
Member

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.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread stdlib/src/lib.rs Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread stdlib/src/locale.rs Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread stdlib/src/locale.rs Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread stdlib/src/locale.rs Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread stdlib/src/locale.rs Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread stdlib/src/locale.rs Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread stdlib/src/locale.rs Resolved

Uh oh!

There was an error while loading. Please reload this page.

@minhrongcon2000
Refactor code
09c750c
@minhrongcon2000
minhrongcon2000 force-pushed the fix/add-localeconv-function branch from c73148b to 09c750c Compare February 24, 2023 14:42
fanninpm
fanninpm reviewed Feb 24, 2023
View reviewed changes
Comment thread stdlib/src/locale.rs Resolved

Uh oh!

There was an error while loading. Please reload this page.

@minhrongcon2000
Fix mismatch typing
844a30a
@minhrongcon2000
minhrongcon2000 requested review from fanninpm and youknowone and removed request for fanninpm February 24, 2023 16:41
@minhrongcon2000
Use libc::c_char instead
26f103a
fanninpm
fanninpm reviewed Feb 24, 2023
View reviewed changes
Comment thread Lib/test/test_locale.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

fanninpm
fanninpm reviewed Feb 24, 2023
View reviewed changes
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,
};

@fanninpm fanninpm Feb 24, 2023

Copy link
Copy Markdown
Contributor

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.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@minhrongcon2000 minhrongcon2000 Feb 24, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

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.

Uh oh!

There was an error while loading. Please reload this page.

youknowone reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
minhrongcon2000 added 8 commits February 25, 2023 01:17
@minhrongcon2000
Fix skip test reason for locale
9374005
@minhrongcon2000
Add setlocale function
757545f
@minhrongcon2000
Merge commit '757545fe37c347c75a5734e0fa9ef9d13a3bcce8' into fix/add-…
184f891
…localeconv-function
@minhrongcon2000
Fix clippy requirements
ecc21e8
@minhrongcon2000
Fix rustfmt issues
973d2b2
@minhrongcon2000
Remove test_locale skip testcase
2d37933
@minhrongcon2000
Fix unittest
082d1ef
@minhrongcon2000
Fix clippy issues
f5730c4
youknowone
youknowone requested changes Feb 25, 2023
View reviewed changes
Comment thread stdlib/src/lib.rs Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread stdlib/src/locale.rs Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread stdlib/src/locale.rs Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread stdlib/src/locale.rs Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread stdlib/src/locale.rs Resolved

Uh oh!

There was an error while loading. Please reload this page.

youknowone
youknowone reviewed Feb 25, 2023
View reviewed changes
Comment thread stdlib/src/locale.rs Resolved

Uh oh!

There was an error while loading. Please reload this page.

@youknowone

youknowone commented Feb 25, 2023

Copy link
Copy Markdown
Member

test_setlocale_category is also successful!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

minhrongcon2000 added 2 commits February 26, 2023 00:06
@minhrongcon2000
Clean up locale module code
31941a4
@minhrongcon2000
Remove setlocale test expected failure
0c5ecb5
@youknowone

youknowone commented Feb 25, 2023

Copy link
Copy Markdown
Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@minhrongcon2000

minhrongcon2000 commented Feb 26, 2023

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@youknowone youknowone added the z-ls-2023 Tag to track Line OSS Sprint 2023 label Feb 26, 2023
@minhrongcon2000

minhrongcon2000 commented 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!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

youknowone
youknowone reviewed Feb 26, 2023
View reviewed changes
Comment thread Lib/test/test_locale.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Lib/test/test_types.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

@minhrongcon2000
Skip format and strcol test case
26f04e2
@minhrongcon2000
minhrongcon2000 force-pushed the fix/add-localeconv-function branch from 77af913 to 26f04e2 Compare February 28, 2023 05:53
minhrongcon2000 added 2 commits February 28, 2023 13:19
@minhrongcon2000
Fix skpTest bug
0268af5
@minhrongcon2000
Skip locale test for window
44a36af
@minhrongcon2000
minhrongcon2000 requested a review from youknowone February 28, 2023 07:33
youknowone
youknowone requested changes Feb 28, 2023
View reviewed changes
Comment thread Lib/test/test_locale.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Lib/test/test_locale.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

@minhrongcon2000
Fix test locale skip convention
9801409
youknowone
youknowone requested changes Feb 28, 2023
View reviewed changes
Comment thread Lib/test/test_locale.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Lib/test/test_locale.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Lib/test/test_locale.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Lib/test/test_locale.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Lib/test/test_types.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

@minhrongcon2000
Revert original skip decorator and polish code
fcfa35a
fanninpm
fanninpm reviewed Feb 28, 2023
View reviewed changes
Comment thread Lib/test/test_locale.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

@minhrongcon2000
Allow test_strcoll_3303
a44a593
fanninpm
fanninpm reviewed Feb 28, 2023
View reviewed changes
Comment thread Lib/test/test_locale.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

@minhrongcon2000
Expect failure instead of skip
9e5be11
youknowone
youknowone reviewed Feb 28, 2023
View reviewed changes
Comment thread Lib/test/test_locale.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

@youknowone
Update Lib/test/test_locale.py
6291ca9
fanninpm
fanninpm reviewed Feb 28, 2023
View reviewed changes
Comment thread Lib/test/test_locale.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

@youknowone
clean up and additional error handling for NulError
38d2253
youknowone
youknowone reviewed Feb 28, 2023
View reviewed changes
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");

@youknowone youknowone Feb 28, 2023

Copy link
Copy Markdown
Member

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.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
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);

@youknowone youknowone Feb 28, 2023

Copy link
Copy Markdown
Member

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.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
youknowone
youknowone approved these changes Feb 28, 2023
View reviewed changes

@youknowone youknowone left a comment •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@youknowone
youknowone merged commit af1d46f into RustPython:main Feb 28, 2023
@youknowone youknowone mentioned this pull request Mar 30, 2025
Feat: locale-module #4081
Closed
@youknowone youknowone mentioned this pull request Dec 25, 2025
locale support including _locale and time.tzname #3850
Closed
1 of 2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fanninpm fanninpm fanninpm left review comments
@youknowone youknowone youknowone approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

z-ls-2023 Tag to track Line OSS Sprint 2023

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@minhrongcon2000 @youknowone @fanninpm
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.