Skip to content

Add week timeframe to Korean locale #804

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

Merged
merged 4 commits into from
Jun 16, 2020
Merged

Conversation

comfuture
Copy link
Contributor

It fixes #808 that weeks timeframe can not be formatted in korean.
And it also adds special plurlized words in korean.

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

Related: #803
Closes: #803

It fixes arrow-py#808 that weeks timeframe can not be formatted in korean.
And it also adds special plurlized words in korean.
Copy link

@ChangJoo-Park ChangJoo-Park left a comment

Choose a reason for hiding this comment

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

@crsmithdev , I reviewed this PR. 

Please merge this.

@systemcatch systemcatch changed the title Fixes korean locale. Add week timeframe to Korean locale Jun 9, 2020
Copy link
Collaborator

@systemcatch systemcatch left a comment

Choose a reason for hiding this comment

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

Hi @comfuture and @ChangJoo-Park, thank you for the contribution, it's always nice to have extra translations by native speakers.

We need test coverage for the changes you've made, once this is done I see no impediment to merging.

@systemcatch systemcatch requested a review from krisfremen June 9, 2020 10:28
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.

@comfuture Thank you for the locale fixes and submitting this MR!

I've added some comments, mainly for more test coverage, as we have been trying to improve the test coverage of all the locales. Other than that, looks excellent!

Cheers!

- add special forms for day/year frame in korean
- add tests to pass code coverage
@codecov-commenter
Copy link

codecov-commenter commented Jun 9, 2020

Codecov Report

Merging #804 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #804   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           10        10           
  Lines         1740      1757   +17     
  Branches       295       300    +5     
=========================================
+ Hits          1740      1757   +17     
Impacted Files Coverage Δ
arrow/locales.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 0a37fa5...3701aca. Read the comment docs.

@comfuture
Copy link
Contributor Author

@krisfremen Thank you for your kind review of my clumsy contributions. I just improved the code by realizing where I was misunderstanding the behavior of locale handling internal methods.

In addition, I added special Korean date/year frame expression. Although there is a feeling of excessiveness compared to other locales, the expressions commonly used in Windows and OS X.

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.

@comfuture great work, just a few more things before it's all good to go.

@systemcatch
Copy link
Collaborator

I'll leave it for @krisfremen to approve but I'm happy with the PR.

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.

@comfuture excellent!

Thank you once again!

Copy link
Collaborator

@systemcatch systemcatch left a comment

Choose a reason for hiding this comment

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

🚀

@systemcatch systemcatch merged commit 451a3fe into arrow-py:master Jun 16, 2020
@Yiyiyimu Yiyiyimu mentioned this pull request Oct 27, 2021
5 tasks
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.

Korean local can broken when humanize() n weeks timeframe.
5 participants