-
Notifications
You must be signed in to change notification settings - Fork 1.3k
'n' support for float format #4865
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
Conversation
This will probably need tests before it gets accepted, but I could not successfully run the tests in Lib/test/test_format.py ... I used |
I believe you can use the following command to test your changes locally: # this runs all of the tests (not necessary if you know which test is affected)
cargo run --release --features ssl,jit -- -m test -j 1 -u all --slowest --fail-env-changed -v
# this runs only the test suite named "test_whatever" (usually located at `Lib/test/test_whatever.py`)
cargo run --release --features ssl,jit -- -m test -j 1 -u all --slowest --fail-env-changed -v test_whatever For a list of all resources and more command-line options, you can execute |
To format the Rust code, try running |
Something's not quite right with either the tests I added for the N case or the action. I'm not getting local test failures at least. |
Fixed tests and N case functionality
I think I got everything figured out. Thanks for your help. A lot of my issues were because of my dev environment but I got them sorted out. The issue with the test was caused by my assuming Python 3.11 supported the N float formatter code but it doesn't. Removed the functionality for that and the test should yield the same Error Py 3.11 does when N is used. |
there's a passing test now in UNEXPECTED SUCCESS: test_format_enum_float (test.test_enum.TestEnum.test_format_enum_float) there's also a panic in the formatting, looking at the runner logs it seems to be due to something being off during a slicing operation. This test doesn't run on Windows (hence why you did not probably see this error). It blows up in |
Thank you for the feedback! I think I fixed it and I also unmarked the unexpected success |
You also fixed more test
and another failure
|
Sorry I keep missing things. Can't run the tests on my dev environment so I have a wonky setup at the moment. Think I got them sorted out. I allowed the N case to function for int formatting functionality when that wasn't supposed to happen. I did not want to introduce "Case" to the Number enum but I wanted the error messages to line up properly with Python 3.11. Hope this is okay |
Generally good idea to push here and wait for results since other platforms might have issues you can't catch, you only need to be able to parse the logs (which is straight-forward after doing it once or twice but can get slightly tedious). |
Glad to hear that, hopefully I won't hit your GitHub action minute limit haha. Can't recall if there's only limitations in private repos or not. Forgot to run |
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 contributing!
@JazzGlobal If you are interested in supporting locale, you can start with #4609 |
Thank you, and thank you all for guiding me through my first Rust contribution 😁 |
Contributes to #1656
I've only recently sprouted my Rustacean legs so let me know if there are better ways to achieve this. I'm sure there will be!
yields