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

Improve date handling #118

Merged
merged 13 commits into from
Jul 6, 2021
Merged

Improve date handling #118

merged 13 commits into from
Jul 6, 2021

Conversation

eljhkrr
Copy link
Member

@eljhkrr eljhkrr commented Jun 18, 2021

Resolves #20 and #124

package.json Show resolved Hide resolved
Copy link
Member

@binokaryg binokaryg left a comment

Choose a reason for hiding this comment

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

Looks good, just left a few minor comments.

src/dateUtils.js Outdated Show resolved Hide resolved
src/dateUtils.js Outdated Show resolved Hide resolved
src/dateUtils.js Outdated Show resolved Hide resolved
src/dateUtils.js Outdated Show resolved Hide resolved
Copy link
Member

@kennsippell kennsippell left a comment

Choose a reason for hiding this comment

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

Lookin' good

src/dateUtils.js Show resolved Hide resolved
package.json Show resolved Hide resolved
src/harness.js Outdated
} else { // shorthand is for days
now += amount * 24 * 60 * 60 * 1000;
}
now = addDate(now, amount);
Copy link
Member

Choose a reason for hiding this comment

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

seems this addDate function is only used once. it is also only one line. I'd recommend removing it from the utils and just having the line here

Copy link
Member Author

Choose a reason for hiding this comment

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

Aiming for separation of concerns such that date implementation is isolated in dateUtils and if we may need to update it later on, it can be done in one place

Copy link
Member

Choose a reason for hiding this comment

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

my opinion is that Luxon should be our "date utility" for 99%+ of functionality. i hope we can get out of the business of providing bespoke date utilities or providing any custom date interfaces, etc. luxon does a better job than we can hope to achieve

this dateUtils is largely for backward compatibility for the old interface

src/dateUtils.js Outdated Show resolved Hide resolved
src/dateUtils.js Outdated Show resolved Hide resolved
src/dateUtils.js Outdated Show resolved Hide resolved
src/dateUtils.js Outdated Show resolved Hide resolved
@eljhkrr eljhkrr requested review from kennsippell, binokaryg and derickl and removed request for eliaswalyba June 28, 2021 17:52
test/harness.spec.js Outdated Show resolved Hide resolved
src/dateUtils.js Outdated Show resolved Hide resolved
src/dateUtils.js Show resolved Hide resolved
test/harness.spec.js Show resolved Hide resolved
@eljhkrr eljhkrr merged commit 39fe990 into master Jul 6, 2021
@eljhkrr eljhkrr deleted the 20_date_improvements branch July 6, 2021 07:21
@kennsippell
Copy link
Member

@eljhkrr Will you npm publish? Do you have permission?

@eljhkrr
Copy link
Member Author

eljhkrr commented Jul 7, 2021

Please share publish permissions

@medic-ci
Copy link

medic-ci commented Apr 9, 2024

🎉 This PR is included in version 1.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

harness.flush() does not consider DST changes when moving forward in days
5 participants