-
Notifications
You must be signed in to change notification settings - Fork 637
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
deprecation(semver): rename eq()
, neq()
, lt()
, lte()
, gt()
and gte()
#4083
Conversation
Co-authored-by: Asher Gomez <ashersaupingomez@gmail.com>
Co-authored-by: Asher Gomez <ashersaupingomez@gmail.com>
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.
LGTM. Thank you!
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 |
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 |
Another concern about naming: We have 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 |
@iuioiua
|
SGTM. To me, |
@kt3k how should we proceed? |
Co-authored-by: Asher Gomez <ashersaupingomez@gmail.com>
Co-authored-by: Asher Gomez <ashersaupingomez@gmail.com>
done |
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 made a couple of tweaks:
- Moved deprecation notices to after their respective JSDoc descriptions. This is needed for deprecation directives to show up properly in IDEs, etc.
- Added an "s" to the "*OrEqual()
functions, to be consistent with
equals()`.
Now, LGTM! Great addition, Tim. Thank you.
Mostly looks good to me. Thanks for updating.
Does that sound better to you? If I search |
I just thought "a equals b". |
|
Maybe not a big deal, but I'd like to hear some more from community members |
I know I said the names don't have to be consistent with Either way, the names convey the purpose of the functions, which is the objective of a function name, with or without the "s". |
TBH I didn't mind the abbreviated names. But I don't mind the longer names either. I vote for |
Well, |
That's true. But if we go for |
I now think it is worth simply aligning with |
Done |
Let's settle on I changed |
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.
LGTM
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.
LGTM
Successor of #4048
Refs: #3948 (comment), #4048 (comment)
less_or_equal_test.ts