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.

Sign up for GitHub

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

Jump to bottom

#8: calculating weeks in year with tests. #12

Merged

franleplant merged 2 commits into franleplant:master from babruix:master

Oct 15, 2016

Contributor

babruix commented Oct 14, 2016

No description provided.


          franleplant#8: calculating weeks in year refactoring and tests.

bc798ba

coveralls commented Oct 14, 2016

Coverage increased (+0.4%) to 84.337% when pulling bc798ba on babruix:master into 4539f9f on franleplant:master.

Owner

franleplant commented Oct 14, 2016

Thanks! Ill take a look tomorrow

On Thu, Oct 13, 2016, 21:18 Coveralls notifications@github.com wrote:

[image: Coverage Status] https://coveralls.io/builds/8330888

Coverage increased (+0.4%) to 84.337% when pulling bc798ba
bc798ba
on babruix:master into 4539f9f
4539f9f
on franleplant:master.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#12 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABxEJ_zl5Uy9KuWw59tXNCMc1bIKn9KWks5qzspKgaJpZM4KWg5X
.

Owner

franleplant commented Oct 14, 2016

This is looking super cool, thanks a lot!
There only remains a few cosmetic changes, please push new commits to this branch and PR instead of closing and opening a new one.

Thanks a bunch, have a nice one!
Fran


          franleplant#8: better solution to get number of weeks in a year.

5a5bf61

coveralls commented Oct 14, 2016

Coverage increased (+0.07%) to 84.049% when pulling 5a5bf61 on babruix:master into 4539f9f on franleplant:master.

franleplant approved these changes

View reviewed changes

src/officialValidators/week.js

-              // TODO: this needs to be improved since some years have 52 weeks
-              const MAX_WEEK = 53
-              export default function weekValidator(control) {
+              function weekValidator(control) {

Owner

franleplant Oct 14, 2016

I would like to keep the export default function weekValidator

src/officialValidators/week.js

		}

		function weeksInYear(year) {

Owner

franleplant Oct 14, 2016

change this to export function weeksInYear

src/officialValidators/week.js

+                return getWeekNumber(dayOfTheMonth)[1];
+              }
+              function getWeekNumber(d) {

Owner

franleplant Oct 14, 2016

change this to export function getWeekNumber

src/officialValidators/week.js

+                return [date.getFullYear(), weekNo];
+              }
+              export {weekValidator as default, weeksInYear, getWeekNumber}

Owner

franleplant Oct 14, 2016

And finally remove this

src/officialValidators/week.js

+                }
+                const dayOfTheMonth = lastDayOfTheYear.setDate(24);
+                return getWeekNumber(dayOfTheMonth)[1];

Owner

franleplant Oct 14, 2016

Sorry to be minimalist here, but do you have any comments on what's happening here?

franleplant merged commit 35d4dd6 into franleplant:master

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment