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

Update french resources and add unit tests #155

Closed
wants to merge 6 commits into from
Closed

Update french resources and add unit tests #155

wants to merge 6 commits into from

Conversation

ygrenier
Copy link
Contributor

Hi,
You will find a few changes :

  • Added missing translations in resources
  • DateHumanizeTests updated to be able to run it on a non-English computer
  • Added unit tests for the French translations

All running on my France french (fr-FR) computer.

I also implements of all changes for the Belgian french resources already defined in project. Normally there is no différences for this parts, but perhaps a validation by a Belgian people is needed.

Best regards,

@@ -3,8 +3,10 @@

namespace Humanizer.Tests
{
public class DateHumanizeTests
public class DateHumanizeTests : AmbientCulture
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for this

@MehdiK
Copy link
Member

MehdiK commented Apr 10, 2014

After your PR a few PRs have been merged into the upstream. Please kindly fetch the latest and rebase your work on the top. Please don't merge your changes.

Also add your PR to the release-notes file.

@ygrenier
Copy link
Contributor Author

Hi,

Excuse me, it's my first PR :)

For the redundants tests I just apply the tests like I found in 'es' or 'de' cultures.
So I remove all of them ?

OK for rebase instead of merge.

Thanks,

@MehdiK
Copy link
Member

MehdiK commented Apr 10, 2014

No need for apology. It's all cool.

You're right about es and de having the same issues. They must've slipped under my radar. Sorry about the confusion.

Opened #163 and #164 to deal with those :)

Force the culture to 'en' for running unit tests on a non-english
computer.
Add missing translations.
Add DateHumanize and TimeSpanHumanize unit tests for 'fr' and 'fr-BE'
culture.
@ygrenier
Copy link
Contributor Author

Hmm I think I make a mistake with my rebase ! There are a lot of commits pushed !

I follow the github help for syncing fork, using rebase instead of merge !

@ygrenier
Copy link
Contributor Author

OK apparently my branch is reseted.

Sorry I'm a SVN and TFS user, git is new for me.

@MehdiK
Copy link
Member

MehdiK commented Apr 10, 2014

I think you may have removed your tests! I cannot see anything.

@MehdiK
Copy link
Member

MehdiK commented Apr 10, 2014

Hey, this is an easy git tutorial. Thought you might find it useful.

@ygrenier
Copy link
Contributor Author

Thanks for the link, it's useful and it's translated in french :)

Yes I removed all unit tests. Have I misunderstood the request ?

@MehdiK
Copy link
Member

MehdiK commented Apr 10, 2014

The unit tests are needed - no PR is accepted to the repo without enough test coverage; but you were testing a lot of cases for each time unit, for example 1, 10, 44, 45, 119 and 120 for minutes, and that was unnecessary. You only needed to test single and plural cases; e.g. 1 minute and 5 minutes.

Please write some tests for both locales and for DateTime and TimeSpan

@ygrenier
Copy link
Contributor Author

Ha ok, sorry. I was suprised to remove all my tests ;)

No problem, I looking for that tonight after job in few hours..

@MehdiK
Copy link
Member

MehdiK commented Apr 11, 2014

Rebased and merged. Thanks for the great effort.

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

No problem, thank for your pateince.
I looking for a NumberToWords for french.

@MehdiK
Copy link
Member

MehdiK commented Apr 11, 2014

Thanks for the contribution. This is now available on NuGet in v1.20.2.

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