Skip to content

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

Closed
Borzoo wants to merge 8 commits into
Humanizr:masterfrom
Borzoo:master
Closed

Added Farsi support to DateTime resource files and NumberToWords.#121
Borzoo wants to merge 8 commits into
Humanizr:masterfrom
Borzoo:master

Conversation

@Borzoo

@Borzoo Borzoo commented Apr 2, 2014

Copy link
Copy Markdown
Contributor

No description provided.

@MehdiK

MehdiK commented Apr 2, 2014

Copy link
Copy Markdown
Collaborator

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.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I love this :)

@Borzoo

Borzoo commented Apr 2, 2014

Copy link
Copy Markdown
Contributor Author

Alright. Sorry about the mistakes.

@MehdiK

MehdiK commented Apr 2, 2014

Copy link
Copy Markdown
Collaborator

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

Borzoo commented Apr 3, 2014

Copy link
Copy Markdown
Contributor Author

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.

Comment thread release_notes.md

Copy link
Copy Markdown
Collaborator

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

MehdiK commented Apr 3, 2014

Copy link
Copy Markdown
Collaborator

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

Borzoo commented Apr 3, 2014

Copy link
Copy Markdown
Contributor Author

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