-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
creating unit conversions functions #2433
Conversation
3b12e73
to
679a0cd
Compare
@LucasHFS As well, could you please add more tests to your test files like |
There was a problem hiding this 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.
@tan75 I get it, can I use this logic in similar situations, e.g. minutesToHours & monthsToYears? |
Yes, you can. |
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
quartersToMonths
Review quartersToYears
Review secondsToHours
Review secondsToMilliseconds
Review secondsToMinutes
Review yearsToMonths
Review yearsToQuarters
There was a problem hiding this 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!
3127586
to
7da59cb
Compare
There was a problem hiding this 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) | ||
}) | ||
}) |
There was a problem hiding this comment.
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).
hi @LucasHFS Hope all is well. Just wondering if you are still working on this PR? |
@tan75 Yep, I'll send the requested changes today. |
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`
Attempt to solve #732 . This is my first PR to the lib, any changes needed let me know.