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

deprecation(semver): rename eq(), neq(), lt(), lte(), gt() and gte() #4083

Merged
merged 26 commits into from
Jan 23, 2024

Conversation

timreichen
Copy link
Contributor

@timreichen timreichen commented Jan 4, 2024

Successor of #4048

Refs: #3948 (comment), #4048 (comment)

  • renames comparator abbreviated functions to the full name
  • deprecates abbreviated functions
  • adds less_or_equal_test.ts

semver/equals.ts Outdated Show resolved Hide resolved
semver/eq.ts Show resolved Hide resolved
timreichen and others added 2 commits January 4, 2024 02:19
Co-authored-by: Asher Gomez <ashersaupingomez@gmail.com>
Co-authored-by: Asher Gomez <ashersaupingomez@gmail.com>
@timreichen timreichen mentioned this pull request Jan 4, 2024
13 tasks
@timreichen
Copy link
Contributor Author

@iuioiua We talked about #4082 and you said single file exports are overkill for cases. So I thought, maybe we should put these functions inside compare.ts as well, since they are sugary compare() functions.

Copy link
Collaborator

@iuioiua iuioiua left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you!

@iuioiua
Copy link
Collaborator

iuioiua commented Jan 4, 2024

@iuioiua We talked about #4082 and you said single file exports are overkill for cases. So I thought, maybe we should put these functions inside compare.ts as well, since they are sugary compare() functions.

Yes. In cases where functions don't incur any dependency costs on each other and share much overlap, it might make sense to have them in the same file. With equals() and friends, this might make sense to do. It also might not. E.g. if such a structure would be surprising to a newcomer, it might not make sense to do. WDYT, @kt3k?

semver/eq.ts Outdated Show resolved Hide resolved
@kt3k
Copy link
Member

kt3k commented Jan 4, 2024

@iuioiua

Yes. In cases where functions don't incur any dependency costs on each other and share much overlap, it might make sense to have them in the same file. With equals() and friends, this might make sense to do. It also might not. E.g. if such a structure would be surprising to a newcomer, it might not make sense to do. WDYT, @kt3k?

Sounds interesting/reasonable idea to me, but not immediately obvious if it's good or not. I'd like to keep discussing that in somewhere else.

@timreichen
Copy link
Contributor Author

@iuioiua

Yes. In cases where functions don't incur any dependency costs on each other and share much overlap, it might make sense to have them in the same file. With equals() and friends, this might make sense to do. It also might not. E.g. if such a structure would be surprising to a newcomer, it might not make sense to do. WDYT, @kt3k?

Sounds interesting/reasonable idea to me, but not immediately obvious if it's good or not. I'd like to keep discussing that in somewhere else.

I'll add it to the list in #3948

@kt3k
Copy link
Member

kt3k commented Jan 4, 2024

Another concern about naming: We have assertLess assertGreater assertGreaterOrEqual assertLessOrEqual in std/assert #3496

So these names might look inconsistent with them. What do you think?

@timreichen
Copy link
Contributor Author

Another concern about naming: We have assertLess assertGreater assertGreaterOrEqual assertLessOrEqual in std/assert #3496

So these names might look inconsistent with them. What do you think?

Thank you for bringing that up. I think we should align to assert names, I'll update the PR.

@iuioiua
Copy link
Collaborator

iuioiua commented Jan 4, 2024

Another concern about naming: We have assertLess assertGreater assertGreaterOrEqual assertLessOrEqual in std/assert #3496
So these names might look inconsistent with them. What do you think?

Thank you for bringing that up. I think we should align to assert names, I'll update the PR.

Sorry for coming in late. I don't see a reason to align these function names with those from std/assert. They're different APIs with different purposes. But more importantly, I think removing the "than" word from these function names decreases their clarity. E.g. the purpose of less() in std/semver isn't immediately obvious. However, lessThan() in std/semver is clearer. What do we think?

@kt3k
Copy link
Member

kt3k commented Jan 5, 2024

@iuioiua
What do you think about the below?

  • lessThan
  • lessOrEqual
  • greaterThan
  • greaterOrEqual

@iuioiua
Copy link
Collaborator

iuioiua commented Jan 5, 2024

@iuioiua What do you think about the below?

  • lessThan
  • lessOrEqual
  • greaterThan
  • greaterOrEqual

SGTM. To me, lessThanOrEqual() and friends sound better. But that's a weak opinion and not something I'd block any PR over. I also understand that lessThanOrEqual() may be too long.

@timreichen
Copy link
Contributor Author

SGTM. To me, lessThanOrEqual() and friends sound better. But that's a weak opinion and not something I'd block any PR over. I also understand that lessThanOrEqual() may be too long.

@kt3k how should we proceed?

@timreichen
Copy link
Contributor Author

@kt3k how should we proceed?

The below pattern looks best among candidates to me.

  • lessThan
  • lessThanOrEqual
  • greaterThan
  • greaterThanOrEqual

done

Copy link
Collaborator

@iuioiua iuioiua left a comment

Choose a reason for hiding this comment

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

I made a couple of tweaks:

  1. Moved deprecation notices to after their respective JSDoc descriptions. This is needed for deprecation directives to show up properly in IDEs, etc.
  2. Added an "s" to the "*OrEqual()functions, to be consistent withequals()`.

Now, LGTM! Great addition, Tim. Thank you.

@kt3k
Copy link
Member

kt3k commented Jan 9, 2024

Mostly looks good to me. Thanks for updating.

Added an "s" to the *OrEqual() functions, to be consistent with equals().

Does that sound better to you? If I search lessThanOrEqual and lessThanOrEquals in google, the former returns more results (lessThanOrEqual looks more common as an API name to me). I think equal here is more of an adjective rather than a verb..

@iuioiua
Copy link
Collaborator

iuioiua commented Jan 9, 2024

I just thought "a equals b".

@kt3k
Copy link
Member

kt3k commented Jan 9, 2024

  • a equals b <- equals is a verb, so it has s suffix
  • a is less than or equal b <- less than and equal are adjectives. equal doesn't have s

@kt3k
Copy link
Member

kt3k commented Jan 9, 2024

lessThanOrEqual or lessThanOrEquals

Maybe not a big deal, but I'd like to hear some more from community members

@iuioiua
Copy link
Collaborator

iuioiua commented Jan 9, 2024

I know I said the names don't have to be consistent with std/assert, but now, the names are consistent with std/assert 😂

Either way, the names convey the purpose of the functions, which is the objective of a function name, with or without the "s".

@Leokuma
Copy link

Leokuma commented Jan 10, 2024

TBH I didn't mind the abbreviated names. But I don't mind the longer names either.

I vote for lessThanOrEquals. To be very precise grammatically, it should be lessThanOrEqualTo, which is too long. So, if we're going to be grammatically imprecise anyway, lessThanOrEquals is semantically closer to lessThanOrEqualTo than lessThanOrEqual is because it keeps a strong sense of direction (which means the order of the arguments in this case).

@timreichen
Copy link
Contributor Author

TBH I didn't mind the abbreviated names. But I don't mind the longer names either.

I vote for lessThanOrEquals. To be very precise grammatically, it should be lessThanOrEqualTo, which is too long. So, if we're going to be grammatically imprecise anyway, lessThanOrEquals is semantically closer to lessThanOrEqualTo than lessThanOrEqual is because it keeps a strong sense of direction (which means the order of the arguments in this case).

Well, lessThanOrEqualTo is just one letter longer than lessThanOrEquals:) I think the length should not be the determining factor, because everybody is probably using autocomplete anyway.

@Leokuma
Copy link

Leokuma commented Jan 11, 2024

That's true. But if we go for lessThanOrEqualTo, maybe we should also rename equals to equalTo. It feels a little too verbose to me. And when I see the word equals, my brain parses it as a symbol rather than a "grammatical word", so it's actually faster for me to read lessThanOrEquals than lessThanOrEqualTo. So I'm still in favor lessThanOrEquals. Sorry if I'm overthinking this 😅.

@kt3k kt3k added the feedback welcome We want community's feedback on this issue or PR label Jan 11, 2024
@iuioiua
Copy link
Collaborator

iuioiua commented Jan 15, 2024

I now think it is worth simply aligning with std/assert. The names are short and simple and show their purpose. That's all that's needed. I'm sorry for the indecisiveness.

@timreichen
Copy link
Contributor Author

I now think it is worth simply aligning with std/assert. The names are short and simple and show their purpose. That's all that's needed. I'm sorry for the indecisiveness.

Done

@kt3k
Copy link
Member

kt3k commented Jan 19, 2024

Let's settle on greaterOrEqual and lessOrEqual for gte and lte replacements, which now have much support from core team (@mmastrac @satyarohith @lucacasonato @crowlKats @dsherret are in favor of them).

I changed less and greater to lessThan and greaterThan. They are a bit inconsistent with assertLess and assertGreater of std/assert, but less & greater look too less descriptive to me. I believe this change is a reasonable compromise between consistency and descriptiveness.

Copy link
Collaborator

@iuioiua iuioiua left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@kt3k kt3k left a comment

Choose a reason for hiding this comment

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

LGTM

@kt3k kt3k merged commit 0a32c8f into denoland:main Jan 23, 2024
12 checks passed
@timreichen timreichen deleted the semver_rename_compare_functions branch July 2, 2024 06:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feedback welcome We want community's feedback on this issue or PR semver
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants