calendar
calendar copied to clipboard
Naming differences for `isAfter` and `isBefore`
Describe the bug
If found another difference in naming when it comes to isAfter
vs isGreaterThen
and isAfterOrEqual
vs isGreaterThanEq
.
I think the difference is even greater then the previous one as we have an additional Then
and Eq
instead of Equal
.
Expected behavior
I would expect that the naming follows the same schema so that all are using the same schema.
I'd prefer isAfter
and isAfterOrEqual
and the same for isBefore
.
Additional context
Is there a reason for the naming or is it just something that fell down before the initial release? If so, I would provide a pull request to handle it.
Hey! Let me first bring some more context into this In general isAfter/isAfterOrEqual should be used with following classes:
- DateTime 🆗
- Time ❌
- Day 🆗
- Month 🆗
- Year 🆗
isGreater and isGreaterThanEq should be used with:
- TimeUnit 🆗
So there are actually 2 problems here (unless I missed something):
- small inconsistency between isGreaterThanEq and isAfterOrEqual (Eq vs Equal)
- Time should follow the same convention as DateTime/Day/Month/Year
The reason why isGreater is used in TimeUnit is that it's hard to say that "1 second is after/before 5 seconds" which works perfectly fine when talking about DateTime/Time/Day/Month/Year. Unification is one thing, but code should be as close as possible to natural language IMO.
The reason why I used Eq and not Equal was the total length of the method name, but now I'm not so sure if that was the right move.
That makes perfect sense. I just stumbled over it when using $commandExecutedAt->isAfter(..
and changing that to a local time check with $commandExecutedAt->time()->is...
where the naming was suddenly different.
So I would prepare a PR like the last one with the two changes you described in the same way as last time where I deprecate the old usages and pipe them through to the updated naming. Does that work for you?
Sounds good!
On that note, to prevent double deprecation: What do you think about renaming isEqual
to isEqualTo
.
isGreaterThan
also uses a preposition and I think isEqual
should also include one. What do you think?
I'm more attached to isEqual than isGreaterThan so personally, I would do it another way around and end up with isGreater but it's a very subjective opinion, honestly I'm not sure what to do here
I just googled and the most relevant thing I found is this: https://english.stackexchange.com/questions/71259/is-equal-to-or-equals
Apparently the only two "correct" ones in English seem to be equals
and isEqualTo
.
So I think the most "correct" thing would be to change it to:
-
isEqualTo
-
isAfterOrEqualTo
-
isGreaterThenOrEqualTo
More to read, but I think easier to understand as it's more exact.
But I've seen isEqual
in the wild often enough that I don't think it's wrong and in the end it is your library, so I would just adapt it in the way you decide 🙂
Then lets maybe stick to the one that is correct in English, isEqualTo as you suggested, your research makes good point and I'm not that attached after all.
Awesome 🎉 It's a pleasure to work with people that put the system / developer experience first 🙂
I will open up a PR as soon as I've gotten through all changes.