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

Allow hyphenated ranges in compatability specs #1410

Merged
merged 1 commit into from
Nov 19, 2019
Merged

Allow hyphenated ranges in compatability specs #1410

merged 1 commit into from
Nov 19, 2019

Conversation

DilumAluthge
Copy link
Member

Fixes #843
Closes #1232

This pull request implements @KristofferC's suggestion in this PR comment.

cc: @KristofferC @StefanKarpinski @00vareladavid @fredrikekre @goretkin @tkf

Copy link
Member

@KristofferC KristofferC left a comment

Choose a reason for hiding this comment

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

This feels like a "feature" that we might want to think about backporting to 1.x

@codecov
Copy link

codecov bot commented Sep 22, 2019

Codecov Report

Merging #1410 into master will increase coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1410      +/-   ##
==========================================
+ Coverage   87.27%   87.31%   +0.03%     
==========================================
  Files          25       25              
  Lines        5383     5398      +15     
==========================================
+ Hits         4698     4713      +15     
  Misses        685      685
Impacted Files Coverage Δ
src/versions.jl 93.62% <100%> (+0.5%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 50f9c6c...6054482. Read the comment docs.

@DilumAluthge
Copy link
Member Author

This feels like a "feature" that we might want to think about backporting to 1.x

I wouldn't oppose that, but I am curious why you think we should backport this.

@KristofferC
Copy link
Member

Because otherwise packages that want to support this have to drop the LTS version of julia. Maybe it isn't a big deal.

Copy link
Member

@fredrikekre fredrikekre left a comment

Choose a reason for hiding this comment

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

Just some doc comments.

docs/src/compatibility.md Show resolved Hide resolved

```toml
[compat]
PkgA = "0.1 - 0.2.3" # [0.1, 0.2.3]
Copy link
Member

Choose a reason for hiding this comment

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

Given https://discourse.julialang.org/t/how-can-we-encourage-julia-package-developers-to-release-version-1-0-0/29124, maybe one way is to use post 1.0 numbers in examples like this? 😄 At least in addition, to the current example.

docs/src/compatibility.md Outdated Show resolved Hide resolved
docs/src/compatibility.md Outdated Show resolved Hide resolved
docs/src/compatibility.md Show resolved Hide resolved
@timholy
Copy link
Member

timholy commented Oct 2, 2019

I like this. To fully obsolete #1349, this would also need to document 0.7-*.

@StefanKarpinski
Copy link
Member

A concern that should be considered is if we want to support pre-release versions then 1.2.3-4.5.6 is both a valid version range and a pre-release of 1.2.3 with pre-release tuple (4, 5, 6). See https://github.com/npm/node-semver#prerelease-tags.

@StefanKarpinski
Copy link
Member

We could just disambiguate that as a version range, which seems like the more common case, although then there isn’t any way to write the pre-release version, whereas if we interpreted this as the pre-release version the version range can still be written as 1.2.3 - 4.5.6 since that is clearly not a single version number.

@KristofferC
Copy link
Member

Maybe enforce having a space on the sides of the hypen for now?

@StefanKarpinski
Copy link
Member

Would it be nuts to use a different character than - which already means something in the actual standardized semver syntax? We could write 1.2.3:4.5.6 for version ranges. The : character has no meaning in single-version syntax. That's a bit of an unfortunate mismatch with the registry range syntax, but 🤷‍♂

@DilumAluthge
Copy link
Member Author

Would it be nuts to use a different character than - which already means something in the actual standardized semver syntax? We could write 1.2.3:4.5.6 for version ranges. The : character has no meaning in single-version syntax. That's a bit of an unfortunate mismatch with the registry range syntax, but 🤷‍♂

1.2.3:4.5.6 seems kind of hard to read. So many dots! How about using the word to? Then you’d have 1.2.3 to 4.5.6.

@StefanKarpinski
Copy link
Member

At that point I'd rather have 1.2.3 - 4.5.6 with required spaces.

@DilumAluthge
Copy link
Member Author

?

@tkf
Copy link
Member

tkf commented Oct 16, 2019

Another option may be --. In LaTeX it's compiled to an en-dash which is used to represent a range of values: https://en.wikipedia.org/wiki/Dash#Ranges_of_values

@oschulz
Copy link

oschulz commented Oct 21, 2019

I wouldn't oppose that, but I am curious why you think we should backport this.

I think with the current push to encourage dependency version limits (upper bounds now required for automatic merging on General), this feature would be very very helpful, and I'd be very glad if it were backported to v1.0!

I just ran into a situation where this package resolution failed, because I upgraded "ElasticArrays" to v1.0 and updated another package to require it - but another dependency that I wanted required "ElasticArrays" v0.2. I can work around that by just setting "ElasticArray <= 2.0" on my other package - but what if I need a lower bound as well?

I have a distinct feeling that without the ability to specify deps version ranges, will run into (extremely frustrating) situations in which package resolution fails more and more often, as package authors put tighter bounds on dependency versions.

@StefanKarpinski
Copy link
Member

this feature would be very very helpful, and I'd be very glad if it were backported to v1.0!

As a general rule, features aren't backported. Pkg is a bit special though, and I've been toying around with the idea of making releases of 1.0 with an updated version of Pkg.

@oschulz
Copy link

oschulz commented Oct 22, 2019

@fredrikekre reminded me that it's already possible to specify a comma-separated list of versions -somehow I missed that or forgot about it, sorry! So since there's a decent workaround, I'll have take back my earlier statement about this needing a backport to 1.0.

@DilumAluthge
Copy link
Member Author

I completely forgot about this pull request. What was the latest decision? Enforce a space on either side of the hyphen? I.e. 1.2.3 - 4.5.6 is allowed, but 1.2.3-4.5.6 is not allowed?

@DilumAluthge
Copy link
Member Author

Alright, I have updated the PR with all of the review comments.

I am enforcing the rule that there must be whitespace on both sides of the hyphen. That way, there is no confusion/ambiguity with prerelease notation.

I think this is ready to merge. cc: @StefanKarpinski @KristofferC @fredrikekre

@DilumAluthge
Copy link
Member Author

DilumAluthge commented Nov 10, 2019

Bump @StefanKarpinski @KristofferC @fredrikekre.

Where did we land on this PR? Do we want to merge it? Or do we want to close it and push users towards using caret and tilde notation instead?

@DilumAluthge
Copy link
Member Author

Bump. Any update on merging this?

@00vareladavid
Copy link
Contributor

FWIW node seems to use the same "space padded hyphen" notation for ranges: https://docs.npmjs.com/misc/semver

Also, the tests seem pretty comprehensive.

@KristofferC KristofferC merged commit 8f97b18 into JuliaLang:master Nov 19, 2019
@DilumAluthge DilumAluthge deleted the da/range-compat-spec branch November 19, 2019 23:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

semver syntax: support hyphen ranges
8 participants