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

Udpate French locale to match the new V2 API #778

Merged
merged 4 commits into from Jun 26, 2018

Conversation

Lakston
Copy link
Contributor

@Lakston Lakston commented Jun 12, 2018

There are some problems with the tests and I'm bad at regex so I'll have to work more on those.

edit: forgot to mention the associated issue: #777

Copy link
Contributor

@leshakoss leshakoss left a comment

Choose a reason for hiding this comment

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

Thanks a lot for your contribution! 🙏 Please address my review comments

@@ -1,5 +1,42 @@
import buildFormatLongFn from '../../../_lib/buildFormatLongFn/index.js'
Copy link
Contributor

Choose a reason for hiding this comment

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

Please fix the code style issues: https://travis-ci.org/date-fns/date-fns/jobs/391123642#L487

/home/travis/build/date-fns/date-fns/src/locale/fr/_lib/formatLong/index.js
  40:5  error  'formatLong' is already defined  no-redeclare
  
/home/travis/build/date-fns/date-fns/src/locale/fr/_lib/localize/index.js
  74:1  error  Expected indentation of 4 spaces but found 6  indent
  
/home/travis/build/date-fns/date-fns/src/locale/fr/_lib/match/index.js
  15:1   error  More than 1 blank line not allowed  no-multiple-empty-lines
  50:25  error  Unnecessary escape character: \'    no-useless-escape
  
/home/travis/build/date-fns/date-fns/src/locale/fr/test.js
   48:35  error  Strings must use singlequote        quotes
   80:27  error  Strings must use singlequote        quotes
  122:1   error  More than 1 blank line not allowed  no-multiple-empty-lines

// assert(result === '10h, 10h, 10h')
// })

// removed the 3rd assertion, AFAIK there are no narrow versions of midnight in French
Copy link
Contributor

Choose a reason for hiding this comment

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

Please return the third assertion back but let's use the same minuit for it. See: https://www.unicode.org/cldr/charts/32/summary/fr.html#1962

})

// France is using a 24h day, there are no AM PM notions
// it('AM, PM', function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's reenable the test. Even if France doesn't use the AM/PM, format, we still should add it for consistency. Using just AM and PM is OK: https://www.unicode.org/cldr/charts/32/summary/fr.html#1928

// France is using a 24h day, there are no AM PM notions
// it('AM, PM', function () {
// var result = format(date, 'h a, h aaaa, haaaaa', {locale: locale})
// assert(result === '10h, 10h, 10h')
Copy link
Contributor

Choose a reason for hiding this comment

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

Please, instead of commenting out some tests, use it.skip, so the testing utility will clearly show what functionality is yet to be implemented

@Lakston
Copy link
Contributor Author

Lakston commented Jun 15, 2018

Thanks for the review ! Oh lord this made me realise I kind of commited some garbage files, sorry for that ! :)

I looked into the AM / PM some more in French language and it appears that the rules state that we use it if necessary and always capitalise it so I reflected that in the last commit (as seen here in the unicode chart)

Test is now un commented, same for 'minuit' which should be use in all cases.

Hope this helps up move forward a bit more !

@Lakston
Copy link
Contributor Author

Lakston commented Jun 15, 2018

Managed to narrow it down to the test errors that I can't understand where the result yields weird values such as:

assert(result === 'demain à 7:30')
               |      |                    
               |      false                
               "demain à 4513070200000"   

and the parsing tests which I have to work on because my regex game is weak.

Copy link
Contributor

@leshakoss leshakoss left a comment

Choose a reason for hiding this comment

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

I'll try to run the tests later to see what is wrong. Meanwhile, please fix the formatRelative. Thanks!

today: '[aujourd’hui à] LT',
tomorrow: '[demain à] LT',
nextWeek: 'dddd [à] LT',
lastWeek: "dddd 'dernier à' LT",
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use eeee instead of dddd; p instead of LT; P instead of L. Those are the format tokens: https://date-fns.org/v2.0.0-alpha.10/docs/format we've changed them in 8th alpha

@leshakoss
Copy link
Contributor

@Lakston I fixed some of the tests (see the last commit in your branch). Could you please verify that my changes make sense in French? Thanks

Copy link
Contributor

@leshakoss leshakoss left a comment

Choose a reason for hiding this comment

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

@Lakston I am merging the PR. Please check the last commit anyway :-)

@leshakoss leshakoss merged commit 13f17df into date-fns:master Jun 26, 2018
@Lakston
Copy link
Contributor Author

Lakston commented Jun 26, 2018

I started doing that yesterday :)

I'll have some modifications but I'll make another PR probably because I have some questions to ask first, I'll post those in the related issue.

@kossnocorp
Copy link
Member

The locale is released as 2.0.0-alpha.11, thank you for your work!

@Lakston Lakston deleted the i18n/french-locale branch June 27, 2018 06:25
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.

None yet

3 participants