-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Add time comparison guideline #357
base: master
Are you sure you want to change the base?
Conversation
README.adoc
Outdated
[source,ruby] | ||
---- | ||
# bad | ||
created_at < 5.minutes.ago |
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.
A concern I have is that people are used to <
in a SQL query involving dates, and so it feels natural to use it in Rails too.
I agree that before?/after?
is a better and more natural API but it might be a step too far to mandate this.
README.adoc
Outdated
shutdown_at.after?(1.minute.from_now) | ||
---- | ||
|
||
Use `past?` and `future?` when comparing with the current time. |
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.
👍 totally agree with this part.
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.
I'm fine with doing the prime time for past?
/future?
, and just briefly mentioning before?
/after?
below the main section with no guidance to use. WDYT?
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.
Agreed.
past?
/future?
Noteable offenders:
Rails docs:
future?
past?
Secondary, informative section:
before?
after?
Introduction of
before?
/after?
in Rails 6.0.before?
/after?
didn't make to this guideline due to insufficient use, and it would go against our: