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

Add partial isequal for VersionNumber #42780

Conversation

IanButterworth
Copy link
Member

@IanButterworth IanButterworth commented Oct 23, 2021

Instead of

julia> v1 = v"1.6"; v2 = v"1.6.3";

julia> v1.major == v2.major && v1.minor == v2.minor
true

this provides

julia> isequal(v"1.6", v"1.6.3", downto = :minor)
true

julia> isequal(v"1.6", v"1.6.3", downto = :patch)
false

julia> isequal(v"1.6", v"1.6.3")
false

@IanButterworth IanButterworth force-pushed the ib/versionnumber_partial_isequal branch from 4a4ec47 to 1a9a2de Compare October 23, 2021 22:03
@IanButterworth
Copy link
Member Author

@StefanKarpinski I'd appreciate a review. In particular I didn't want to add something that would be considered bad for semver best practice

@IanButterworth IanButterworth force-pushed the ib/versionnumber_partial_isequal branch from 1a9a2de to 302c36b Compare November 7, 2021 06:54
@IanButterworth
Copy link
Member Author

Given no red flags have been raised I'll go ahead

@IanButterworth IanButterworth merged commit c1f94b6 into JuliaLang:master Nov 7, 2021
@IanButterworth IanButterworth deleted the ib/versionnumber_partial_isequal branch November 7, 2021 20:17
@mcabbott
Copy link
Contributor

mcabbott commented Nov 7, 2021

I am far from an expert on semver, but this seems a bit weird. I hoped a real expert would chime in.

What seems odd to me is that this is symmetric. If you have x = v"1.6", for what purpose would you want to accept both v"1.5" and v"1.7" equally? I would have thought the more common thing to want is an asymmetric check, which would accept only things on one side, thus v"1.7"but not v"2.1" (or v"1.5" but not v"0.9"), which > doesn't do.

@KristofferC
Copy link
Member

KristofferC commented Nov 7, 2021

Sorry for post merge review, but I find it kind of strange to add specific keyword arguments to an extended generic function. You can only use the keyword argument if you know the specific type but then why should the generic function be extended at all. The generic function should be documentable and there is no way to document the keyword arguments anymore since they are depending on concrete implementations.

This could perhaps be its own function that doesn't extend anything else.

@IanButterworth
Copy link
Member Author

Ok. Seems I was too eager here for a feature that's only a slight convenience. I'll revert.

@StefanKarpinski
Copy link
Member

Didn't see this, but I'm glad it's reverted. In general, I object to isequal ever acting in a non-transitive way, which this patch would have implemented. Note that you can do this with the thismajor, thisminor, thispatch functions, e.g. thisminor(v1) == thisminor(v2). We could export those functions potentially.

LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Feb 22, 2022
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Mar 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants