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

creating unit conversions functions #2433

Merged
merged 36 commits into from May 28, 2021

Conversation

LucasHFS
Copy link
Contributor

Attempt to solve #732 . This is my first PR to the lib, any changes needed let me know.

@LucasHFS LucasHFS force-pushed the 732-unit-conversion-functions branch from 3b12e73 to 679a0cd Compare April 20, 2021 12:25
@tan75 tan75 self-assigned this Apr 20, 2021
@tan75 tan75 self-requested a review April 22, 2021 19:54
src/daysToWeeks/test.ts Outdated Show resolved Hide resolved
@tan75
Copy link
Contributor

tan75 commented Apr 27, 2021

@LucasHFS
Thanks a lot for your work!
Could you please fix all the examples in your files as per this commit below:
Commit

As well, could you please add more tests to your test files like
daysToWeeks(13); - see the comment from @wenderjean
From my point of view, I would expect the result of the above call to be 1

Copy link
Contributor

@tan75 tan75 left a comment

Choose a reason for hiding this comment

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

Thank you for your PR! Please, address my comments.

@LucasHFS
Copy link
Contributor Author

From my point of view, I would expect the result of the above call to be 1

@tan75 I get it, can I use this logic in similar situations, e.g. minutesToHours & monthsToYears?

@tan75
Copy link
Contributor

tan75 commented Apr 28, 2021

From my point of view, I would expect the result of the above call to be 1

@tan75 I get it, can I use this logic in similar situations, e.g. minutesToHours & monthsToYears?

Yes, you can.
I think it makes sense.

@LucasHFS LucasHFS requested a review from tan75 April 28, 2021 18:25
tan75 added 16 commits May 2, 2021 14:01
Review hoursToMilliseconds
Review of hoursToMinutes
Review of hoursToSeconds
Review of millisecondsToHours
Review millisecondsToMinutes
Review of millisecondsToSeconds
Review of minuteToHours
Review minutesToMilliseconds
Review minutesToSeconds
Review of monthsToQuarters
Review monthsToYears
Review quartersToYears
Review secondsToHours
Review secondsToMilliseconds
Copy link
Contributor

@tan75 tan75 left a comment

Choose a reason for hiding this comment

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

Many thanks for your PR!

@kossnocorp kossnocorp force-pushed the 732-unit-conversion-functions branch from 3127586 to 7da59cb Compare May 14, 2021 04:27
Copy link
Member

@kossnocorp kossnocorp left a comment

Choose a reason for hiding this comment

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

Thank you so much for your great work!

The only problem I found in the PR is that test examples focus on what a test does instead of the function behavior. Please rewrite those using the example I added for millisecondsToMinutes.

Please rewrite tests

assert(millisecondsToMinutes(60000.5) === 1)
assert(millisecondsToMinutes(0) === 0)
})
})
Copy link
Member

Choose a reason for hiding this comment

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

Please take a look at this test file. I want you to rewrite tests in the PR to make them look like this.

Instead of focusing on what test does converts 700 milliseconds to seconds I want you describe behavior and test it. It's enough to have a couple of similar examples that test the same thing:

assert(millisecondsToMinutes(60000) === 1)
assert(millisecondsToMinutes(120000) === 2)

Also, when a function does rounding, I want you to add an example for that (please make sure that the first test doesn't check for rounding).

And finally, test border values (float and zero).

@tan75
Copy link
Contributor

tan75 commented May 23, 2021

hi @LucasHFS

Hope all is well. Just wondering if you are still working on this PR?

@LucasHFS
Copy link
Contributor Author

@tan75 Yep, I'll send the requested changes today.

@tan75 tan75 requested a review from kossnocorp May 25, 2021 16:39
@kossnocorp kossnocorp merged commit a0f660c into date-fns:master May 28, 2021
kossnocorp pushed a commit that referenced this pull request May 28, 2021
Added:

- Added 18 new conversion functions:
  - `daysToWeeks`
  - `hoursToMilliseconds`
  - `hoursToMinutes`
  - `hoursToSeconds`
  - `millisecondsToHours`
  - `millisecondsToMinutes`
  - `millisecondsToSeconds`
  - `minutesToHours`
  - `minutesToMilliseconds`
  - `minutesToSeconds`
  - `monthsToQuarters`
  - `monthsToYears`
  - `quartersToMonths`
  - `quartersToYears`
  - `secondsToHours`
  - `secondsToMilliseconds`
  - `secondsToMinutes`
  - `weeksToDays`
  - `yearsToMonths`
  - `yearsToQuarters`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants