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 Comparator.semver property #4059

Closed

Conversation

timreichen
Copy link
Contributor

@timreichen timreichen commented Jan 2, 2024

  • deprecates Comparator.semver property in favour of Comparatorextending SemVer.

@timreichen timreichen requested a review from kt3k as a code owner January 2, 2024 16:29
@github-actions github-actions bot added the semver label Jan 2, 2024
@iuioiua
Copy link
Contributor

iuioiua commented Jan 2, 2024

What is the reasoning behind this?

@timreichen
Copy link
Contributor Author

What is the reasoning behind this?

Imo separating Operator and SemVer by individual properties is just unnecessary nesting.

@iuioiua iuioiua changed the title deprecation(semver): deprecate Operator.semver property deprecation(semver): deprecate Comparator.semver property Jan 3, 2024
@iuioiua
Copy link
Contributor

iuioiua commented Jan 3, 2024

I'm not sure about this change. It'd require many changes to the std/semver API, and its benefits aren't apparent.

@timreichen
Copy link
Contributor Author

timreichen commented Jan 3, 2024

I'm not sure about this change. It'd require many changes to the std/semver API, and its benefits aren't apparent.

I think it declutters the interface. It also will simplify SemVerRange, and once we redefine SemVerRange without the ranges property, it will look much more clean:

before:

const range = { ranges: [ [ { operator: "<", semver: { major: 1, minor: 2, patch: 3 } } ] ] }

after

const range = [ [ { operator: "<", major: 1, minor: 2, patch: 3 } ] ]

But we can wait with this until we decide about #4047 and discuss this some more.

@iuioiua
Copy link
Contributor

iuioiua commented Jan 3, 2024

But we can wait with this until we decide about #4047 and discuss this some more.

Yeah, I think we should do that.

@kt3k
Copy link
Member

kt3k commented Jan 8, 2024

I think we should first decide on what to do about #4047 before changing anything about Comparator

@timreichen
Copy link
Contributor Author

I think we should first decide on what to do about #4047 before changing anything about Comparator

This is about the type Comparator which will stay public either way because it is part of SemVerRange.

@kt3k
Copy link
Member

kt3k commented Jan 8, 2024

This is about the type Comparator which will stay public either way because it is part of SemVerRange.

I think we can stop using Comparator in SemVerRange definition if we only want to change SemVerRange

@timreichen
Copy link
Contributor Author

timreichen commented Jan 8, 2024

This is about the type Comparator which will stay public either way because it is part of SemVerRange.

I think we can stop using Comparator in SemVerRange definition if we only want to change SemVerRange

How do you mean exactly?

@kt3k
Copy link
Member

kt3k commented Jan 9, 2024

For example, by replacing Comparator in SemVerRangeAnd like the below:

type SemVerRangeAnd = (SemVer & { operator: Operator })[];

@iuioiua
Copy link
Contributor

iuioiua commented Jan 10, 2024

Does this PR still apply if Comparator is now deprecated?

@timreichen
Copy link
Contributor Author

Does this PR still apply if Comparator is now deprecated?

I will push another PR where we can do that step when renaming SemVerRange to Range directly. Closing.

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