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

Date range constructor automatically sorts start/end dates #201

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Date range constructor automatically sorts start/end dates #201

wants to merge 3 commits into from

Conversation

TristanJM
Copy link
Contributor

@TristanJM TristanJM commented Dec 4, 2017

DateRanges should have start date before the end date, this is now automatically corrected in the constructor.

Should a user need a reverse range, they can easily change their application logic to remember that start/end is the opposite.

Closes #106

@TristanJM TristanJM changed the title Date range constructor automatically sorts start/end dates Date range constructor automatically sorts start/end dates (Issue 106) Dec 4, 2017
@TristanJM TristanJM changed the title Date range constructor automatically sorts start/end dates (Issue 106) Date range constructor automatically sorts start/end dates (Issue https://github.com/rotaready/moment-range/issues/106) Dec 4, 2017
@TristanJM TristanJM changed the title Date range constructor automatically sorts start/end dates (Issue https://github.com/rotaready/moment-range/issues/106) Date range constructor automatically sorts start/end dates (Issue #106) Dec 4, 2017
Copy link
Contributor

@gf3 gf3 left a comment

Choose a reason for hiding this comment

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

👍 lgtm, don't forget to update the CHANGELOG

@gf3 gf3 added feature and removed enhancement labels Jan 19, 2018
@gf3 gf3 changed the title Date range constructor automatically sorts start/end dates (Issue #106) Date range constructor automatically sorts start/end dates Jan 29, 2018

if (this.end.isBefore(this.start)) {
[this.start, this.end] = [this.end, this.start];
}

Choose a reason for hiding this comment

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

This doesn't seem safe to me (e.g. developer calling this makes a typo).

Double-check: have we already considered throwing an error instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for discussing 😃

IMO the library should provide the core time functionality, and the user can easily change their application logic if they're expecting a reversed time range. We classify it as a period between two points in time, rather than a directional period.

Throwing an error isn't ideal because most of the time, the functions on a range behave identically regardless of the start or end. I would think that it's far more inconvenient to require application start/end checks or error handling when they likely aren't requiring reverse time ranges? But I see the issue (if it's not documented clearly) of users not understanding what's happening.

@gf3 do you think it's better to throw error or is it unnecessary?

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.

3 participants