Skip to content
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

Revert changes to TimeZoneName::new parser #1167

Merged
merged 3 commits into from
Jul 7, 2023

Conversation

pitdicker
Copy link
Collaborator

See #1087 (comment).

With the changes to support a minus sign when parsing a timezone offset, #1087 accidentally changed the parsing of a timezone abbreviation in a tzif file. This is just a short ASCII string, that doesn't care about a minus sign.

@jtmoon79
Copy link
Contributor

jtmoon79 commented Jul 4, 2023

Could this PR include another commit to add the test_timezonename_new (adjusted for this code change)?

@djc
Copy link
Contributor

djc commented Jul 4, 2023

Oops, should have noticed this in review.

@pitdicker
Copy link
Collaborator Author

Could this PR include another commit to add the test_timezonename_new (adjusted for this code change)?

I think having a test that mostly is a list of numbers or offset values works a bit confusing. But I've updated the test to some realistic values, and to test the cases TimeZoneName::new checks.

@pitdicker pitdicker force-pushed the tzfile_tzname branch 2 times, most recently from 5890e6d to fe37696 Compare July 4, 2023 10:36
@pitdicker
Copy link
Collaborator Author

Sorry for all the pushes. This is not working from a Windows PC 😆

@pitdicker
Copy link
Collaborator Author

@jtmoon79 Are the changes to the test acceptable?

@pitdicker
Copy link
Collaborator Author

@djc do you want to have a second look at the test, or shall I merge this?

@djc
Copy link
Contributor

djc commented Jul 7, 2023

Thanks for asking, merge away!

@pitdicker pitdicker merged commit 56c6967 into chronotope:0.4.x Jul 7, 2023
33 checks passed
@pitdicker pitdicker deleted the tzfile_tzname branch July 7, 2023 12:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants