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

Standardize locales and improve locale validation #954

Merged
merged 13 commits into from Apr 18, 2021
Merged

Conversation

jadchaar
Copy link
Member

@jadchaar jadchaar commented Apr 8, 2021

Pull Request Checklist

Thank you for taking the time to improve Arrow! Before submitting your pull request, please check all appropriate boxes:

  • 🧪 Added tests for changed code.
  • 🛠️ All tests pass when run locally (run tox or make test to find out!).
  • 🧹 All linting checks pass when run locally (run tox -e lint or make lint to find out!).
  • 📚 Updated documentation for changed code.
  • ⏩ Code is up-to-date with the master branch.

If you have any questions about your code changes or any of the points above, please submit your questions along with the pull request and we will try our best to help!

Description of Changes

Closes: #952

@codecov
Copy link

codecov bot commented Apr 8, 2021

Codecov Report

Merging #954 (989d93c) into master (ac6a204) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #954   +/-   ##
=======================================
  Coverage   99.74%   99.74%           
=======================================
  Files          10       10           
  Lines        1957     1964    +7     
  Branches      313      313           
=======================================
+ Hits         1952     1959    +7     
  Misses          4        4           
  Partials        1        1           
Impacted Files Coverage Δ
arrow/api.py 100.00% <100.00%> (ø)
arrow/arrow.py 99.00% <100.00%> (+<0.01%) ⬆️
arrow/constants.py 100.00% <100.00%> (ø)
arrow/factory.py 100.00% <100.00%> (ø)
arrow/formatter.py 100.00% <100.00%> (ø)
arrow/locales.py 100.00% <100.00%> (ø)
arrow/parser.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ac6a204...989d93c. Read the comment docs.

@jadchaar
Copy link
Member Author

jadchaar commented Apr 8, 2021

@systemcatch @krisfremen do ya'll think I should change the variable that is referenced when we throw errors about locales? E.g. duplicate locale registered in a subclass or if a locale does not exist in a call to get_locale? Currently, we use the original locale they pass in like en_US, but if we are transforming it into en-us for our internal lookup, should we display this in the error message instead?

Copy link
Member

@krisfremen krisfremen left a comment

Choose a reason for hiding this comment

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

Since we are doing a bit of sanitizing anyway, I think what we have in the locales should be fine to return in the errors, it'll help to adopt the new codes also.

@systemcatch
Copy link
Collaborator

I agree with Kris, let's start returning the new codes in errors.

@jadchaar
Copy link
Member Author

Fixed @krisfremen @systemcatch!

Copy link
Member

@krisfremen krisfremen left a comment

Choose a reason for hiding this comment

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

One question from first glance, otherwise looking good.

arrow/api.py Outdated Show resolved Hide resolved
Copy link
Member

@krisfremen krisfremen left a comment

Choose a reason for hiding this comment

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

One more thought off the top of my head, but I'll have to double check this.

README.rst Outdated Show resolved Hide resolved
Copy link
Member

@krisfremen krisfremen left a comment

Choose a reason for hiding this comment

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

Looks good to me. 👍

@jadchaar
Copy link
Member Author

Look good to you @systemcatch?

@systemcatch
Copy link
Collaborator

Yeah Jad looks good to merge.

@jadchaar jadchaar merged commit 1310dbb into master Apr 18, 2021
@jadchaar jadchaar deleted the validate-locales branch April 18, 2021 17:29
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.

Standardize separator and casing used in locale names
3 participants