-
Notifications
You must be signed in to change notification settings - Fork 451
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
Keep @modifiers when parsing locales #947
Conversation
3c3f3e8
to
ae8a175
Compare
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.
Thank you for the contribution!
Some initial thoughts and comments within; you'll probably also need to rebase since #949 just landed...
See python-babel#947 (comment) Signed-off-by: martin f. krafft <madduck@madduck.net>
See python-babel#947 (comment) Signed-off-by: martin f. krafft <madduck@madduck.net>
See python-babel#947 (comment) Signed-off-by: martin f. krafft <madduck@madduck.net>
See python-babel#947 (comment) Signed-off-by: martin f. krafft <madduck@madduck.net>
See python-babel#947 (comment) Signed-off-by: martin f. krafft <madduck@madduck.net>
See python-babel#947 (comment) Signed-off-by: martin f. krafft <madduck@madduck.net>
c10d05f
to
ccca33b
Compare
@akx Thank you for your time. I addressed most of your comments, and left an open question in the above. Happy to proceed any way, just let me know what you prefer. |
See python-babel#947 (comment) Signed-off-by: martin f. krafft <madduck@madduck.net>
See python-babel#947 (comment) Signed-off-by: martin f. krafft <madduck@madduck.net>
See python-babel#947 (comment) Signed-off-by: martin f. krafft <madduck@madduck.net>
See python-babel#947 (comment) Signed-off-by: martin f. krafft <madduck@madduck.net>
See python-babel#947 (comment) Signed-off-by: martin f. krafft <madduck@madduck.net>
See python-babel#947 (comment) Signed-off-by: martin f. krafft <madduck@madduck.net>
ccca33b
to
b5d7749
Compare
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.
Thanks for implementing the suggested changes!
There's a couple more comments within, but I think we're pretty close. Thanks :)
Codecov Report
@@ Coverage Diff @@
## master #947 +/- ##
==========================================
- Coverage 90.91% 90.89% -0.02%
==========================================
Files 25 25
Lines 4327 4340 +13
==========================================
+ Hits 3934 3945 +11
- Misses 393 395 +2
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
See python-babel#947 (comment) Signed-off-by: martin f. krafft <madduck@madduck.net>
See python-babel#947 (comment) Signed-off-by: martin f. krafft <madduck@madduck.net>
See python-babel#947 (comment) Signed-off-by: martin f. krafft <madduck@madduck.net>
See python-babel#947 (comment) Signed-off-by: martin f. krafft <madduck@madduck.net>
See python-babel#947 (comment) Signed-off-by: martin f. krafft <madduck@madduck.net>
See python-babel#947 (comment) Signed-off-by: martin f. krafft <madduck@madduck.net>
f46d266
to
901b60f
Compare
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.
Tests are failing, can you take a look? Thanks.
901b60f
to
0f0ef1f
Compare
Sorry for the failing tests, was due to the 4/5 tuple return depending on |
Locale modifiers ("@Variants") are described in the GNU gettext documentation like this: > The ‘@variant’ can denote any kind of characteristics that is not > already implied by the language ll and the country CC. […] It can also > denote a dialect of the language, … Wherein Babel previously would discard these, this patch stores the modifier information in the `Locale` objects, handling string representation accordingly. Not implemented is the lookup of a meaningful description of modifiers, but instead — for now — an identity mapping is provided. Resolves: python-babel#946 Signed-off-by: martin f. krafft <madduck@madduck.net>
See python-babel#947 (comment) Signed-off-by: martin f. krafft <madduck@madduck.net>
See python-babel#947 (comment) Signed-off-by: martin f. krafft <madduck@madduck.net>
Signed-off-by: martin f. krafft <madduck@madduck.net>
See python-babel#947 (comment) Signed-off-by: martin f. krafft <madduck@madduck.net>
See python-babel#947 (comment) Signed-off-by: martin f. krafft <madduck@madduck.net>
See python-babel#947 (comment) Signed-off-by: martin f. krafft <madduck@madduck.net>
See python-babel#947 (comment) Signed-off-by: martin f. krafft <madduck@madduck.net>
Signed-off-by: martin f. krafft <madduck@madduck.net>
Signed-off-by: martin f. krafft <madduck@madduck.net>
Signed-off-by: martin f. krafft <madduck@madduck.net>
Co-authored-by: Aarni Koskela <akx@iki.fi>
0f0ef1f
to
83188f0
Compare
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.
Thanks, looks good!
Locale modifiers ("@Variants") are described in the GNU gettext documentation like this:
Wherein Babel previously would discard these, this patch stores the modifier information in the
Locale
objects, handling string representation accordingly.Not implemented is the lookup of a meaningful description of modifiers, but instead — for now — an identity mapping is provided.
All tests pass. Furthermore, integration with
flask-babel
has been verified to work.Resolves: #946
Signed-off-by: martin f. krafft madduck@madduck.net