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

Added Farsi support to DateTime resource files and NumberToWords. #121

Closed
wants to merge 8 commits into from

Conversation

Borzoo
Copy link
Contributor

@Borzoo Borzoo commented Apr 2, 2014

No description provided.

@MehdiK
Copy link
Member

MehdiK commented Apr 2, 2014

Great effort. Thanks for this.

Just a few things:

  • You are a fair few commits behind the head. Please rebase your work, resolve the merge conflicts and (force) push up.
  • Please add tests for DateTime.Humanize and TimeSpan.Humanize. You can check out Arabic tests for that.
  • Please move your NumberToWords tests (and the DateTime and TimeSpan ones) to a new folder under Localisation (like Arabic tests).
  • Please use AmbientCulture to enforce culture for your tests. Again you can see existing tests for this under Localisation.
  • Please add your PR to release_notes file on the root of the repo.

[InlineData(123456789, "صد و بیست و سه میلیون و چهارصد و پنجاه و شش هزار و هفتصد و هشتاد و نه")]
[InlineData(1234567890, "یک میلیارد و دویست و سی و چهار میلیون و پانصد و شصت و هفت هزار و هشتصد و نود")]
[Theory]
public void ToWordsFarsi(int number, string expected)
Copy link
Member

Choose a reason for hiding this comment

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

I love this :)

@Borzoo
Copy link
Contributor Author

Borzoo commented Apr 2, 2014

Alright. Sorry about the mistakes.

@MehdiK
Copy link
Member

MehdiK commented Apr 2, 2014

No need for apology. This is just a coding standard which I have to enforce to make sure the codebase stays clean and consistent considering all the PRs that's coming through :)

Thanks for the great work. Looking forward to merging this in.

@Borzoo
Copy link
Contributor Author

Borzoo commented Apr 3, 2014

I've fixed my mistakes. I sense that the way I use git is a little messy. If so, any guidance will be greatly appreciated as this is my first encounter with git.

@@ -1,3 +1,9 @@
###v1.16.1 - 2014-04-03
- [#121] (https://github.com/MehdiK/Humanizer/pull/121) : Added Farsi support to DateTime and NumberToWords
Copy link
Member

Choose a reason for hiding this comment

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

We still don't know what version this is going to or when this is going to be released. So new features should be added under 'In Development'

@MehdiK
Copy link
Member

MehdiK commented Apr 3, 2014

This is great stuff; but a bit too involved as it touches many things. So the first guidance: always keep your PRs as minimal and focused as possible. Please create two separate PRs: one for adding Farsi to existing localisable code and another for ToQuantity and supporting changes. Second guidance: don't work on master. Always branch per feature.

I am glad you made small commits. You should be able to cherrypick the needed commits out of your current working folder. To do this you can:

git checkout -b localised-ToQuantity
git checkout -b farsi-localisation
git reset hard 598ba9efa5
// make changes to the code based on the feedback

git checkout master
git reset --hard HEAD~9 // or however many commits you're off the upstream head
git pull upstream master // or whatever you've called your upstream remote so you can cleanly apply upstream changes on top of your master

git rebase master farsi-localisation // rebasing on top of upstream work
git push origin farsi-localisation
// build and run tests to make sure it's still in good shape
// create a new PR & close this one (as this is on master)

Your ToQuantity work is going to hurt a bit to cherry pick. So let's get the farsi localisation out of the way first, and we will look into the other one later.

@Borzoo
Copy link
Contributor Author

Borzoo commented Apr 3, 2014

Alright. Thanks.

@Borzoo Borzoo closed this Apr 3, 2014
@Borzoo Borzoo mentioned this pull request May 28, 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