Skip to content
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

Attempt at refactoring to_currency into base #135

Merged
merged 12 commits into from
Nov 9, 2017

Conversation

shulcsm
Copy link
Contributor

@shulcsm shulcsm commented Nov 6, 2017

Status

  • READY
  • HOLD
  • WIP (Work-In-Progress)

Additional notes

Attempt at generalizing and moving to_currency down as discussed in #133
If this is fine i can do it to rest of mentioned languages.

However i'm not happy with Num2Word_Base and how it works. I'm not sure how to do rest of integration. What is the purpose of all these initialization callbacks? Shouldn't static things be better off as static properties?

@coveralls
Copy link

coveralls commented Nov 6, 2017

Coverage Status

Coverage increased (+0.3%) to 82.76% when pulling f181a26 on shulcsm:refactor_base into 5a131fe on savoirfairelinux:master.

Copy link
Collaborator

@erozqba erozqba left a comment

Choose a reason for hiding this comment

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

This looks promising we could end having fewer files, less code and providing the same functionalities. We don't really need en_EUR, en_GB, en_IN, es_CO, es_VE, fr_DZ, that were added to provide to_currency() with others currencies.

You should use static properties if that seems more appropriate to you, just let's try to make the code more "DRY" and remove the duplication!

Thanks a lot for your work and your time!
It's really appreciated!


return self.to_splitnum(val, hightxt="euro/s", lowtxt="cents",
jointxt=jointxt, longval=longval, cents=cents)
pass


n2w = Num2Word_EN_EUR()
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can now remove this file! :)


return self.to_splitnum(val, hightxt="pound/s", lowtxt="pence",
jointxt="and", longval=longval)
pass


n2w = Num2Word_EN_GB()
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can now remove this file! :)

@coveralls
Copy link

coveralls commented Nov 7, 2017

Coverage Status

Coverage increased (+3.7%) to 86.177% when pulling 495002f on shulcsm:refactor_base into 5a131fe on savoirfairelinux:master.

@coveralls
Copy link

coveralls commented Nov 7, 2017

Coverage Status

Coverage increased (+4.01%) to 86.438% when pulling 6dcedbc on shulcsm:refactor_base into 5a131fe on savoirfairelinux:master.

@coveralls
Copy link

coveralls commented Nov 7, 2017

Coverage Status

Coverage increased (+4.07%) to 86.495% when pulling 98cd874 on shulcsm:refactor_base into 5a131fe on savoirfairelinux:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+4.07%) to 86.495% when pulling 98cd874 on shulcsm:refactor_base into 5a131fe on savoirfairelinux:master.

@shulcsm shulcsm changed the title Attempt at refactoring lv to base Attempt at refactoring to_currency into base Nov 7, 2017
@coveralls
Copy link

coveralls commented Nov 7, 2017

Coverage Status

Coverage increased (+4.6%) to 86.993% when pulling 7231415 on shulcsm:refactor_base into 5a131fe on savoirfairelinux:master.

@shulcsm
Copy link
Contributor Author

shulcsm commented Nov 7, 2017

I had some issues with moving uk doctests. Some of them don't pass, looks like something to do with one and feminine form. Someone native speaking should look into that.

@coveralls
Copy link

coveralls commented Nov 7, 2017

Coverage Status

Coverage increased (+4.8%) to 87.252% when pulling e7212cb on shulcsm:refactor_base into 5a131fe on savoirfairelinux:master.

@shulcsm
Copy link
Contributor Author

shulcsm commented Nov 7, 2017

I think i'm done here for now. Unless you have something to add. There is some duplication left in to_cardinal but i'm not sure what to do with it. I thin base needs some work, maybe after that.

Copy link
Collaborator

@erozqba erozqba left a comment

Choose a reason for hiding this comment

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

Hello @shulcsm,
This looks really good!
Thanks a lot for this awesome work!
We need to handle the case of the test not passing, but first, we need to continue removing the code duplication.

Looking into the git history, the "lang_LT" was there when our college @hsoft move the project to savoirfairelinux.

So my suggestion is made the class 'Num2Word_LT' inherit from NumWord_Base, and the other class Num2Word_LV, Num2Word_PL, Num2Word_UK inherit from 'Num2Word_LT'.

Probably the contributors of Num2Word_LV, Num2Word_PL, Num2Word_UK just take Num2Word_LT as inspiration and copy/paste the code.

What do you think?


return self.to_splitnum(val, hightxt="dollar/s", lowtxt="cent/s",
jointxt="and", longval=longval, cents=True)


n2w = Num2Word_EN()
to_card = n2w.to_cardinal
to_ord = n2w.to_ordinal
to_ordnum = n2w.to_ordinal_num
to_year = n2w.to_year
Copy link
Collaborator

Choose a reason for hiding this comment

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

We could remove this..

@shulcsm
Copy link
Contributor Author

shulcsm commented Nov 7, 2017

I don't want to make inheritance chain deeper and only thing that would solve is get rid of duplication for to_cardinal. _int2word look similar but they are not. Same with pluralize.
If you really want to get rid of to_cardinal i can move it to mixin or something.
IMHO right solution for that would be pushing it down to base and generalizing all implementations but it's out of scope here.

@coveralls
Copy link

coveralls commented Nov 7, 2017

Coverage Status

Coverage increased (+4.8%) to 87.23% when pulling 26aac00 on shulcsm:refactor_base into 5a131fe on savoirfairelinux:master.

@erozqba
Copy link
Collaborator

erozqba commented Nov 9, 2017

@shulcsm OK, you have convinced me, let's not make the inheritance chain deeper... We will try to push it to the base and generalize all implementations. I will only try to find the way or the person to verify the tests and implementation of UK before merge this and break something. I will try to look at this in the next day.
Thanks a lot again!

@shulcsm
Copy link
Contributor Author

shulcsm commented Nov 9, 2017

Ok. Failing tests have nothing to do with changes, though. It was in broken state before I touched it.

@erozqba
Copy link
Collaborator

erozqba commented Nov 9, 2017

@shulcsm if it is the case, then I will trust you here and accept this so we can move forward in fixing the test and pushing to_cadinal to the base class so we could remove the code duplication.

@erozqba erozqba merged commit 1e954c9 into savoirfairelinux:master Nov 9, 2017
@shulcsm
Copy link
Contributor Author

shulcsm commented Nov 9, 2017

Thanks, here is an issue for tests: #138
You can also close some of the coverage issues, they have been solved with this.

@erozqba
Copy link
Collaborator

erozqba commented Nov 9, 2017

👍 thanks @shulcsm

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.

3 participants