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

Changed the NumberToWords Converter to find the right converter by the language Name as well #174

Closed
wants to merge 9 commits into from
Closed

Changed the NumberToWords Converter to find the right converter by the language Name as well #174

wants to merge 9 commits into from

Conversation

akamud
Copy link
Contributor

@akamud akamud commented Apr 11, 2014

Fix suggested by @kappy in Issue #168

This is needed for languages that can be ambiguous in Two Letter ISO Code (pt-BR, pt-PT both have 'pt' Two Letter ISO code).

…e language Name aswell.

This is needed for languages that can be ambiguous in Two Letter ISO Code (pt-BR, pt-PT both have 'pt' Two Letter ISO code).
@akamud
Copy link
Contributor Author

akamud commented Apr 11, 2014

I changed the Converter code to first look for the RFC 1766 standard name, as described here, and then to look for the Two Letter ISO Name.

I added a test and a dummy class for Brazilian Portuguese just to show it can get the factory for both cases.

@akamud akamud changed the title Changed the NumberToWords Converter to find the right converter by the language Name aswell Changed the NumberToWords Converter to find the right converter by the language Name as well Apr 11, 2014
@MehdiK
Copy link
Member

MehdiK commented Apr 11, 2014

Please rebase your work on top of upstream. Your fork is a few commits behind.

[Fact]
public void CanGetRFCStandardLanguageSpecificFactory()
{

Copy link
Member

Choose a reason for hiding this comment

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

Please remove redundant spaces.

@MehdiK
Copy link
Member

MehdiK commented Apr 11, 2014

Thanks for this @akamud and @kappy. Great fix.

Please rebase your work on top of upstream. You're a few commits behind.

MehdiK and others added 6 commits April 11, 2014 10:35
…e language Name aswell.

This is needed for languages that can be ambiguous in Two Letter ISO Code (pt-BR, pt-PT both have 'pt' Two Letter ISO code).
…factories.

Better NumerToWordsFactoryTests formatting.
…om/akamud/Humanizer into feature/to-words-converter-by-name

Conflicts:
	release_notes.md
	src/Humanizer.Tests/Localisation/NumerToWordsFactoryTests.cs
@akamud
Copy link
Contributor Author

akamud commented Apr 11, 2014

Please check if everything is correct now

{
public override string Convert(int number)
{
return "not implemented";
Copy link
Member

Choose a reason for hiding this comment

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

This will upset Brazilian users I believe. We need to implement this first.

MehdiK pushed a commit that referenced this pull request Apr 11, 2014
MehdiK pushed a commit that referenced this pull request Apr 11, 2014
@MehdiK MehdiK mentioned this pull request Apr 11, 2014
@MehdiK
Copy link
Member

MehdiK commented Apr 11, 2014

I rebased your work and tidied up a few things and pushed up to 183. We need to implement the Brazilian converter or we'll upset a few users. Please pull that branch down, implement the converter and push to a new PR.

Closing this now.

@MehdiK MehdiK closed this Apr 11, 2014
@akamud
Copy link
Contributor Author

akamud commented Apr 11, 2014

I'm finishing the pt-BR implementation. Will push when I'm done.

akamud added a commit to akamud/Humanizer that referenced this pull request Apr 12, 2014
akamud added a commit to akamud/Humanizer that referenced this pull request Apr 12, 2014
MehdiK pushed a commit that referenced this pull request Apr 12, 2014
MehdiK pushed a commit that referenced this pull request Apr 12, 2014
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.

2 participants