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): deprecate ltr() and gtr() #4228

Merged
merged 5 commits into from
Feb 5, 2024

Conversation

timreichen
Copy link
Contributor

@timreichen timreichen commented Jan 23, 2024

  • renames ltr() to lessThanRange() and deprecates the old function
  • renames gtr() to greaterThanRange() and deprecates the old function
  • deprecates ltr()
  • deprecates gtr()

@timreichen timreichen requested a review from kt3k as a code owner January 23, 2024 09:08
Copy link
Contributor

@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 agree with the changes. Nice.

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

kt3k commented Jan 26, 2024

I wonder what are the use cases of these functions. I guess most users are only interested in testRange for comparing a version and a range. In what cases, would people try checking these conditions?

@timreichen
Copy link
Contributor Author

I wonder what are the use cases of these functions. I guess most users are only interested in testRange for comparing a version and a range. In what cases, would people try checking these conditions?

I have no idea. I think it is pretty trivial to get the min/max version of a range and compare it to another version. But I guess there must be a reason why it was ported over.

@kt3k
Copy link
Member

kt3k commented Feb 1, 2024

But I guess there must be a reason why it was ported over.

I feel we blindly ported everything from npm:semver and these wasn't a discussion about utilities of these APIs.

I'd suggest we should just deprecate these without replacement APIs because they don't seem common operation and also the alternative ways to perform these are trivial as you point out. The deprecation note would be something like use greaterThan(version, rangeMax(range)) instead

@timreichen timreichen force-pushed the semver_rename_range_comparators branch from e731e6e to dbf1883 Compare February 1, 2024 22:23
@timreichen
Copy link
Contributor Author

But I guess there must be a reason why it was ported over.

I feel we blindly ported everything from npm:semver and these wasn't a discussion about utilities of these APIs.

I'd suggest we should just deprecate these without replacement APIs because they don't seem common operation and also the alternative ways to perform these are trivial as you point out. The deprecation note would be something like use greaterThan(version, rangeMax(range)) instead

done

@kt3k kt3k changed the title Deprecation(semver): rename ltr() to lessThanRange() and gtr() to greaterThanRange() deprecation(semver): deprecate ltr() and gtr() Feb 2, 2024
semver/gtr.ts Outdated Show resolved Hide resolved
semver/ltr.ts Outdated Show resolved Hide resolved
@iuioiua
Copy link
Contributor

iuioiua commented Feb 2, 2024

Towards #4228

Copy link
Contributor

@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. I've made some modifications and opened a new issue (#4273) to track the deprecation. Does this new way of deprecation look good to us?

@kt3k
Copy link
Member

kt3k commented Feb 5, 2024

LGTM. I've made some modifications and opened a new issue (#4273) to track the deprecation. Does this new way of deprecation look good to us?

Thanks! #4273 Looks good to me.

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 a6fe34b into denoland:main Feb 5, 2024
12 checks passed
@timreichen timreichen deleted the semver_rename_range_comparators branch July 2, 2024 06:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants