-
-
Notifications
You must be signed in to change notification settings - Fork 974
refactor(locale): ko state data update #3487
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
✅ Deploy Preview for fakerjs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## next #3487 +/- ##
=======================================
Coverage 99.97% 99.97%
=======================================
Files 2859 2859
Lines 219518 219518
Branches 951 952 +1
=======================================
+ Hits 219463 219465 +2
+ Misses 55 53 -2
🚀 New features to boost your workflow:
|
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.
The change itself looks good. Although, I noticed that all the states are from south Korea only (as they where before). Technically, the locale ko
is not bound to south Korea specifically, but all koran speaking countries. We might need to revisit this in the future and introduce a ko_KR
locale. That way the locale data would more accurately describe its semantic definition.
Since the entries did exist before this change was proposed, I don't see the necessity to do this immediately.
That is also the case for a bunch of other locales like fr and ru and jp and de. So I agree that's a fight for another day 😀 |
Thank you for your contribution @seoahan.
|
Split state changes from street changes PR as requested.
Expanded state names to their full names in
state.ts
.