Number Spelling #114

blagasz · 2014-10-12T10:50:43Z

Reinstantiated pull request to make it a clear changeset.

The spelling framework tries to be as generic as possible, but still structured upon my original non-generic implementation of the Hungarian spelling. The Hungarian language in general and the spelling in particular is very complicated, so I hope that the generic helper structure that is suitable for an easy implementation of the Hungarian spelling would be also suitable for all the other languages.

sils · 2015-07-31T19:10:23Z

@blagasz hey, thanks for taking the time to create a PR! Sorry for taking so long to respond. We're trying to get the project up to speed again, could you rebase your commits onto current master? There seem to be conflicts.

blagasz · 2015-07-31T19:16:23Z

Yes, I'll get down to it soon.

Happy to hear from you :)

Szabolcs
On 31 Jul 2015 21:10, "Lasse Schuirmann" notifications@github.com wrote:

@blagasz https://github.com/blagasz hey, thanks for taking the time to
create a PR! Sorry for taking so long to respond. We're trying to get the
project up to speed again, could you rebase your commits onto current
master? There seem to be conflicts.

—
Reply to this email directly or view it on GitHub
#114 (comment).

blagasz · 2015-08-01T12:37:05Z

Hi @sils1297, I think I managed to rebase first time in my life :)

sils · 2015-08-01T19:34:14Z

@blagasz hey, thanks :)

This PR seems to hold some merge commits and other commits related to how you did the rebase due to conflicts I presume. This makes it hard to review it properly and is not really clean with respect to the history of the project.

You seem to be new to git (or at least not that much into the cool stuff it offers ;)) so let me explain how I think this can be solved best :) It's probably best to reset I'm assuming you have the remote babel pointing at this repository and origin pointing to your fork, if you don't have another remote, just create it with git remote add babel https://github.com/mitsuhiko/babel/.

Checkout your working branch, which is master in your case git checkout master
Fetch from the babel repository: git fetch babel
git diff babel/master should now show only the changes you want to make.
Reset the staging area to the babel master. This will basically reset your git branch to the state of this repositories master while keeping the state of your files: git reset babel/master
A git diff should now show the same diff as in 3.
You can now use git add and git add -p to add whole files or partial files to the staging area and git commit (without -a!) to commit only the staged changes. This way you can create proper logical changeset.
You now need to force push. Note that it is not recommended to do such things on master on production repositories! I don't have time to write a full git tutorial here but please keep that in mind. git push --force origin master
Your clean commits are now on this branch :)

If at any time anything goes wrong - git is your friend and has a history of all recent actions. You can use git reflog to access that history and git reset --hard <commit id> (attention, destructive action!) to reset the staging area as well as your files to that state.

I hope that helps a bit. If you need anything else, ping me.

blagasz · 2015-08-02T10:55:12Z

Hi @sils1297, thanks for the quick tutorial, I have a long way to go with git :)
Hopefully my second try has what you need. Have a nice day!

sils · 2015-08-02T11:28:01Z

@erickwilder can you take a look at this one?
@blagasz cool, that seems to have worked. Could you take a look at http://coala.readthedocs.org/en/latest/Getting_Involved/Writing_Good_Commits/ while we investigate how the code looks. We don't want something with "hopefully works" in the commit message and I think you might find reading this useful.

blagasz · 2015-08-02T12:08:53Z

Sorry about that, it was about the git juggling to clear the commit, not
about the code. I shouldn't have put it in the commit message...

Would you like me to recommit with a better commit message?

On 2 August 2015 at 13:28, Lasse Schuirmann notifications@github.com
wrote:

@erickwilder https://github.com/erickwilder can you take a look at this
one?
@blagasz https://github.com/blagasz cool, that seems to have worked.
Could you take a look at
http://coala.readthedocs.org/en/latest/Getting_Involved/Writing_Good_Commits/
while we investigate how the code looks. We don't want something with
"hopefully works" in the commit message and I think you might find reading
this useful.

—
Reply to this email directly or view it on GitHub
#114 (comment).

sils · 2015-08-02T12:55:30Z

@blagasz yes, please :) git commit --amend will let you change the message, no need to go through the reset stuff again in case you don't know. Please try to follow the guidelines on the site I've given, try to summarize the change in the shortlog (first line) and add more elaborate explanation and reasoning in the body.

sils · 2015-08-02T12:56:59Z

@blagasz really good commit messages are an art, if you plan to work on open source projects you'll probably learn it sooner or later, don't be frustrated about them or people letting you amend the commit message again and again - you're not the only one struggling with this. Good messages are hard to do and it takes some time to learn to do it properly.

blagasz · 2015-08-02T13:32:12Z

@sils1297 thanks for your support and explanation. I'm not frustrated as I know that everyone is doing this for the greater good! I tried to do my best and any further feedback is very welcome.

sils · 2015-08-02T14:30:35Z

@blagasz hey, can you add Fixes https://github.com/mitsuhiko/babel/issues/179 below your commit message? This way we can track the history of your commits via that bug and can access the old review comments which are lost otherwise.

erickwilder · 2015-08-04T14:45:47Z

babel/number_spelling.py

I didn't get why you're not using super() here and let Python's inheritance system do it's job.

@erickwilder I personally prefer explicity in those cases, I see only two uses of super in Babel so we'll probably want to agree on some style on this. The style guide has no information on this.

@sils1297
I don't see super() as an implicit construct of the language. Also, using super() makes easier to do future refactoring that involves changes in the inheritance tree, makes easier to deal with mixins and other very useful patterns.

@erickwilder the thing is super provides only one advantage, that is you don't have to do another renaming when you rename the baseclass. However super has some caveeats especially when using it for multiple inheritance because you have to make clear which arguments are passed where and such things. That being said, super is complex, I'm sure there are many people out there who don't understand it fully (especially in multiple inheritance cases, I also count me to this group of people btw.) while providing only a very small advantage that is neglectible for many users doing renamings via IDEs. We had some discussions about super usage in general at coala and decided collectively against it. I know that opinions on this are split, your call.

What I see is that super() itself is not complex. Multiple inheritance on the other hand is. I can't blame super() for multiple inheritance complexities (kinda like blame the gun by killing someone, not the killer).

If you're gonna change the way we call parent class methods, this should be done consistently across the library and I think that is is not the subject of this pull request. Just to not extend this discussion, I do think that we can use super() here (it's already used, minimally but still used as yourself pointed out) and on a separate thread we can discuss/change the way we handle how to deal with methods overwritten by child classes.

@sils1297 seems reasonable?

Just for the record. super() without arguments only works in Python 3. Python 2 requires two arguments for super().

Highly recommended talk: Super considered super! by Raymond Hettinger

Ok, I will incorporate all the changes. Should I make a new pull request or
still modify the current commit.

I have nothing against super, I decided to use the simple way to make it
simple, but I could add super no problem.

I also add the factoring of the locale specific code and some loading
mechanism.

I'll leave out decimals global context.

Get rid of unused files.

Get back to you as soon as I have the time to finish all that.

Szabolcs
On 4 Aug 2015 22:09, "Isaac Jurado" notifications@github.com wrote:

In babel/number_spelling.py
#114 (comment):

self.value = sum(d.value_dec(10)_*e for e,d in enumerate(self.digits)) # might use int

def len(self):

return len(self.digits)

def iter(self):

for digit in self.digits:

yield digit

+class DigitContext(ContextBase):

"""

Represent one digit of a number and its context

"""

def init(self, value, number, side, group, index):

ContextBase.**init**(self)

Just for the record. super() without arguments only works in Python 3.
Python 2 requires two arguments for super().

Highly recommended talk: Super considered super! by Raymond Hettinger
https://www.youtube.com/watch?v=EiOglTERPEo

—
Reply to this email directly or view it on GitHub
https://github.com/mitsuhiko/babel/pull/114/files#r36235049.

On Tue, Aug 4, 2015 at 10:20 PM, blagasz notifications@github.com wrote:

In babel/number_spelling.py:

self.value = sum(d.value_dec(10)_*e for e,d in enumerate(self.digits)) # might use int

def len(self):

return len(self.digits)

def iter(self):

for digit in self.digits:

yield digit

+class DigitContext(ContextBase):

"""

Represent one digit of a number and its context

"""

def init(self, value, number, side, group, index):

ContextBase.**init**(self)

Ok, I will incorporate all the changes. Should I make a new pull
request or still modify the current commit.

Since @sils1297 and @erickwilder must be having a hard time sweeping all
the staled pull requests, I believe it is easier for them to just
overwrite the current pull request.

At least, that is what I try to do. A combination of git commit --amend or git rebase -i, followed by git push -f.

Isaac Jurado

"The noblest pleasure is the joy of understanding"
Leonardo da Vinci

@etanol exactly, thanks!

@blagasz please change your commit with --amend and the interactive rebase and push force your commits on top. Let me know if you need help.

etanol · 2015-08-04T20:05:03Z

babel/number_spelling.py

This approach is not optimal since it changes the global (per thread) decimal context, which can break user code that relies on decimal context stability, in case a user changes it.

The best way to solve this is to use a decimal context manager. For example:

https://github.com/etanol/babel/commit/7ced0273020427cad3bc321001b64f769cf2f85d

codecov-io · 2015-08-05T03:20:31Z

Current coverage is `83.94%`

Merging #114 into master will increase coverage by +0.82% as of 67a46fe

@@            master    #114   diff @@
======================================
  Files           21      25     +4
  Stmts         3787    4048   +261
  Branches         0       0       
  Methods          0       0       
======================================
+ Hit           3148    3398   +250
  Partial          0       0       
- Missed         639     650    +11

Review entire Coverage Diff as of 67a46fe

Powered by Codecov. Updated on successful CI builds.

erickwilder · 2015-08-06T15:14:53Z

babel/number_spelling/__init__.py

I still think that exceptions should be in their own module, just like babel.exceptions does.

Agreed, this also forces good naming as its not file local.

blagasz · 2015-09-24T15:35:55Z

It is already there in the test_framework.py. It throws a SpellerNotFound error. I could change the name...

sils · 2015-09-24T15:37:27Z

It is already there in the test_framework.py. It throws a SpellerNotFound error. I could change the name...

I don't see any text for that

blagasz · 2015-09-24T15:42:01Z

Sorry I haven't staged that...

On 24 September 2015 at 17:37, Lasse Schuirmann notifications@github.com
wrote:

It is already there in the test_framework.py. It throws a SpellerNotFound
error. I could change the name...

I don't see any text for that

—
Reply to this email directly or view it on GitHub
#114 (comment).

blagasz · 2015-09-24T15:44:04Z

@sils1297 I pushed everything now, sorry for the fuss!

blagasz · 2015-09-24T15:46:53Z

Rebased and line removed :)

sils · 2015-09-24T15:47:19Z

Cool, thanks :) This is growing very good, many revisions -> high quality :)

sils · 2015-09-24T15:48:15Z

Oh @etanol could you please take at least a short look at this? You might have valuable additions.

etanol · 2015-09-24T22:03:29Z

@sils1297 I had this PR merged in my custom branch. However, it seems to have changed slightly since then. I will take a look to the new locale_* submodules as I haven't had the time to figure them out. If there is a more urgent need, go ahead and merge it to master; then I can take a look in two weeks to see if something needs to be amended.

sils · 2015-09-26T16:05:57Z

@blagasz hey, so I begin to like this code more and more. I think it would be good to have a few more tests for some corner cases. The patch coverage (https://codecov.io/github/python-babel/babel/compare/46a234e...7fbf2c5) shows that there are some cases like some exceptions and a break statement and so on missing, I'd love to see a few more tests for those to make this even more future proof. Those exceptions e.g. should be rather easy to test.

sils · 2015-09-26T16:06:52Z

And well there's the conflict again probably because of the CHANGES file so this'll probably reoccur. Maybe wait with the rebase a bit if it's cumbersome for you.

sils · 2015-09-26T16:07:28Z

Leaving marked as scheduled for review because I (or somebody else) still needs another deep look at this code and I can't do that right now.

blagasz · 2015-09-29T11:22:40Z

@sils1297 I was trying to improve the coverage and hope you'll will still like the code after a deeper look. I'm working on the Chinese simplified spelling right now, but I will put it in a separate pull. Also I am planning to write a short tutorial for the framework to help collaboration to produce implementations for other locals quickly.

sils · 2015-09-29T12:28:45Z

@blagasz tests fail. We'll wait with review until this is repaired as this is not eligible for merge as long as any CI fails.

blagasz · 2015-09-29T12:32:10Z

Yes, sorry, I know about the problem, some of the new tests fail on 3.* and
I only tested locally on 2.7. A few minutes and I patch that!

On 29 September 2015 at 14:28, Lasse Schuirmann notifications@github.com
wrote:

@blagasz https://github.com/blagasz tests fail. We'll wait with review
until this is repaired as this is not eligible for merge as long as any CI
fails.

—
Reply to this email directly or view it on GitHub
#114 (comment).

New code almost exclusively resides in babel/numbers/_spelling/. A generic helper structure is created and a reference implementation for Hungarian is provided (locale_hu.py). A draft implementation for English (British) is also given (locale_en.py). The babel/numbers.py became a subpackage and modified to give an interface for new code through the spell_number function. Test suite restructered and amended at tests/numbers/ and tests/test_dates.py was modified due to changes in pytz. Fixes #179

blagasz · 2015-09-29T14:04:56Z

@sils1297 all set :)

mitsuhiko · 2015-10-02T14:12:20Z

I'm a strong -1 on pulling this in. The reason for this is that Babel ultimately only allows two sources of information in the library: It either has to come from the olson database or CLDR. Historically there have been only very few exceptions to this rule and they were made for very specific reasons.

The number spelling is specified in the rbnf rules (see common/rbnf/LANG.xml) in the CLDR dataset and can be implemented through a rule engine. I would welcome an implementation of this, but definitely not a manually maintained for a few languages in Babel. It's a maintenance nightmare and impossible to judge. The only reason Babel works is because people that actually understand what they are doing are maintaining the CLDR dataset :)

sils · 2015-10-02T14:24:06Z

@blagasz I do tend to agree with @mitsuhiko . It is ultimately my fault to not have this stopped earlier.

Still the experience you gained might be very helpful understanding and implementing spelling support using CLDR. Are you still interested in working on #179 ?

blagasz · 2015-10-02T15:10:04Z

@sils1297 and @mitsuhiko no hard feelings at all, but the strong -1 from Armin was a tough one :)

I completely agree with Armin and I was myself a little bit surprised when he said two years ago, that my approach "Sounds like something that can find its way into Babel". I was wondering about the CLDR support and every other possibilities then and I came up with my solution as the only viable alternative. The CLDR rbnf rules sounded interesting but it was insufficient and unimplemented in CLDR two years ago, I checked out the new version and it seems to have improved a lot, now it seems something implementable. It is also my fault that I did not checked that earlier as a long time passed since I started my implementation. Maybe I was in love with my own code :)

Anyway, I started to read the docs for the rule based formatting and I am happy to implement the engine (or transform an existing implementation into Python). I am busier these days, but try to have some advancement in that soon.

Personally I gained a lot by this experience (failure) and I am happy to help in any way in the future.

sils · 2015-10-02T15:25:31Z

@blagasz you can accept the invitation for our org on https://github.com/python-babel so I can assign you to the bug. Many thanks for sticking with us.

A pure Python engine for parsing RBNF rules. The rules are incomplete in many cases, fractional number spelling is hardly supported. Based on an earlier discussion: python-babel#114 and referenced in python-babel#179

A pure Python engine for parsing RBNF rules. The rules are incomplete in many cases, fractional number spelling is hardly supported. Based on an earlier discussion: #114 and referenced in #179

sils added the needs rebase label Jul 31, 2015

sils added status/needs work and removed needs rebase labels Aug 1, 2015

sils added the pending review label Aug 2, 2015

sils changed the title ~~Spelling support~~ Number Spelling Aug 2, 2015

sils removed the status/needs work label Aug 2, 2015

sils mentioned this pull request Aug 2, 2015

Add Number Spelling #179

Open

erickwilder reviewed Aug 4, 2015
View reviewed changes

erickwilder added the status/needs work label Aug 4, 2015

etanol reviewed Aug 4, 2015
View reviewed changes

erickwilder reviewed Aug 6, 2015
View reviewed changes

etanol self-assigned this Sep 26, 2015

sils closed this Oct 2, 2015

sils removed the (3) pending review label Oct 2, 2015

blagasz mentioned this pull request Jul 26, 2019

Number spelling based on the CLDR's RBNF rules #660

Closed

blagasz mentioned this pull request Jul 30, 2020

Number spelling #682

Open

blagasz mentioned this pull request Sep 28, 2025

Rule based number formatting #1232

Open

Number Spelling #114

Number Spelling #114

Uh oh!

Conversation

blagasz commented Oct 12, 2014

Uh oh!

sils commented Jul 31, 2015

Uh oh!

blagasz commented Jul 31, 2015

Uh oh!

blagasz commented Aug 1, 2015

Uh oh!

sils commented Aug 1, 2015

Uh oh!

blagasz commented Aug 2, 2015

Uh oh!

sils commented Aug 2, 2015

Uh oh!

blagasz commented Aug 2, 2015

Uh oh!

sils commented Aug 2, 2015

Uh oh!

sils commented Aug 2, 2015

Uh oh!

blagasz commented Aug 2, 2015

Uh oh!

sils commented Aug 2, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Aug 5, 2015

Current coverage is 83.94%

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blagasz commented Sep 24, 2015

Uh oh!

sils commented Sep 24, 2015

Uh oh!

blagasz commented Sep 24, 2015

Uh oh!

blagasz commented Sep 24, 2015

Uh oh!

blagasz commented Sep 24, 2015

Uh oh!

sils commented Sep 24, 2015

Uh oh!

sils commented Sep 24, 2015

Uh oh!

etanol commented Sep 24, 2015

Uh oh!

sils commented Sep 26, 2015

Uh oh!

sils commented Sep 26, 2015

Uh oh!

sils commented Sep 26, 2015

Uh oh!

blagasz commented Sep 29, 2015

Uh oh!

sils commented Sep 29, 2015

Current coverage is `83.94%`