calendar icon indicating copy to clipboard operation
calendar copied to clipboard

Naming differences for `isAfter` and `isBefore`

Open christian-kolb opened this issue 2 years ago • 8 comments

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.

christian-kolb avatar Aug 16 '22 05:08 christian-kolb

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):

  1. small inconsistency between isGreaterThanEq and isAfterOrEqual (Eq vs Equal)
  2. 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.

norberttech avatar Aug 16 '22 07:08 norberttech

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?

christian-kolb avatar Aug 16 '22 07:08 christian-kolb

Sounds good!

norberttech avatar Aug 16 '22 08:08 norberttech

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?

christian-kolb avatar Aug 16 '22 08:08 christian-kolb

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

norberttech avatar Aug 16 '22 08:08 norberttech

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 🙂

christian-kolb avatar Aug 16 '22 09:08 christian-kolb

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.

norberttech avatar Aug 16 '22 13:08 norberttech

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.

christian-kolb avatar Aug 16 '22 13:08 christian-kolb