-
-
Notifications
You must be signed in to change notification settings - Fork 681
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
Fixing plural of years and days in Hebrew #1042
Conversation
In some languages (Hebrew and Arabic, for example) the plural of years or days are different than the ones until ten
A more correct way to write plural of years and days for number larger than 10 (similar to Arabic above)
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.
Hi @guyernest,
Thanks for the PR.
Let's start with getting the tests to pass first, which needs to have the locale's _format_timeframe to be modified in order to use the new keys.
Take a look at the Arabic locale to see the _format_timeframe implementation, this will probably need to have the "general" keys moved to the new keys.
Thank you for your attention and I apologize for missing these tests failures. |
Codecov Report
@@ Coverage Diff @@
## master #1042 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 10 10
Lines 2166 2168 +2
Branches 344 345 +1
=========================================
+ Hits 2166 2168 +2
Continue to review full report at Codecov.
|
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.
Hi @guyernest,
looks great in terms of code and passing tests!
The only thing left is the code style, if you take a look at the GitHub Actions, there's a few cosmetic fixups needed, you can do all of them automatically by running tox -e lint
.
Other than that, everything else looks good to go!
I didn't notice any change in the files, however, it might be some whitespace characters or new lines that I don't see. I hope now it will pass the workflow smoothly |
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.
@guyernest Thanks for fixing up your PR! That looks great!
Looks good to me!
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.
Looks good to me as well.
Pull Request Checklist
Thank you for taking the time to improve Arrow! Before submitting your pull request, please check all appropriate boxes:
tox
ormake test
to find out!).tox -e lint
ormake lint
to find out!).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