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

Constraints malfunction? #69

Closed
consideRatio opened this issue Apr 6, 2018 · 7 comments
Closed

Constraints malfunction? #69

consideRatio opened this issue Apr 6, 2018 · 7 comments

Comments

@consideRatio
Copy link

consideRatio commented Apr 6, 2018

Since these tests are successful:

{">0", "0.0.1-alpha", true},
{">=0", "0.0.1-alpha", true},

shouldn't the following test also succeed? (They don't)

 {">0.0", "0.0.1-alpha", true}, 
 {">=0.0", "0.0.1-alpha", true}, 
 {">0.0.0", "0.0.1-alpha", true}, 
 {">=0.0.0", "0.0.1-alpha", true}, 
@mh-cbon
Copy link
Contributor

mh-cbon commented Apr 7, 2018

given Y a non PR version string, is its alpha PR version greater than its non PR version, aka IS Y-alpha > Y, the answer is no, i believe, thus : {">Y", "Y-alpha", false},

Because 0.0.1-alpha is greater than 0.0.0/0.0/0, {">0.0", "0.0.1-alpha", true},

Because 0.0.0-alpha is smaller than 0.0.0/0.0/0 {">0.0", "0.0.0-alpha", false},

I believe the test should be updated so we express things in a more natural way:

...

Edit: wrong from me, i totally mixed up, i should have read the tests, given Y a constraint, does the version X validates it. Simply bear in mind that X-alpha < X.

@consideRatio
Copy link
Author

consideRatio commented Apr 7, 2018

given Y a non PR version string, is its alpha PR version greater than its non PR version, aka IS Y-alpha > Y, the answer is no, i believe, thus : {">Y", "Y-alpha", false},

Because 0.0.1-alpha is greater than 0.0.0/0.0/0, {">0.0", "0.0.1-alpha", true},

Because 0.0.0-alpha is smaller than 0.0.0/0.0/0 {">0.0", "0.0.0-alpha", false},

@mh-cbon thanks for clarifying this! This strengthens me to believe the additional test I mentioned should pass. Do you also think they should pass?

# Additional tests *not* in the repo, that fails, and the failure is confusing to me.
{">0.0", "0.0.1-alpha", true}, 
{">0.0.0", "0.0.1-alpha", true}, 

# The test in the repo, that passes, and the pass isn't confusing to me.
{">0", "0.0.1-alpha", true}, 

I believe the test should be updated so we express things in a more natural way:

I appreciate the tests as they are as well, they are close in format to the actual usage of the functions. If I have this constraint, and this version, does the constraint allow it?

@consideRatio
Copy link
Author

Btw, I've ended up here while trying to figure out why writing a constraint like >=v1.8.0 did not match v1.9.4-gke.1. Do you understand why @mh-cbon ?

Apparently it might work if we use >=v1.8.0-asdf or >=v1.8.0-r0, I don't get why either though.

@mh-cbon
Copy link
Contributor

mh-cbon commented Apr 8, 2018

https://github.com/Masterminds/semver#basic-comparisons

Note, according to the Semantic Version specification pre-releases may not be API compliant with their release counterpart. It says,

    A pre-release version indicates that the version is unstable and might not satisfy the intended compatibility requirements as denoted by its associated normal version.

SemVer comparisons without a pre-release value will skip pre-release versions. For example, >1.2.3 will skip pre-releases when looking at a list of values while >1.2.3-alpha.1 will evaluate pre-releases.

I understand now, it has something to do with version pinning, dependency upgrade of some old relations, and beta stuff. I d need a white board to express myself on that matter, but after i read this, i think maintainers are right.

@consideRatio
Copy link
Author

@mh-cbon ah thanks a lot for helping me out!

One question remains for me about why the first case below be different from the others. But it was good to hear that the others were intentionally made that way for a reason I can grasp. I presume it is intentional though since there is actually a test for exactly that so I'm closing this issue.

constraint version constraint.check(version)
">0" "0.0.1-alpha" True (Why this is, I still don't understand)
">0.0" "0.0.1-alpha" False
">0.0.0" "0.0.1-alpha" False

@mh-cbon
Copy link
Contributor

mh-cbon commented Apr 8, 2018

You are welcome.

I took a moment to simply add your questions to the test suite, figured out that i did misread them previously, sorry about that if i mislead you, however, they do not yield the same results as the table you provided.

The test i built now contains :

		{"<=1.1", "1.1.1", false},
		{">0", "0.0.1-alpha", true},
		{">0.0", "0.0.1-alpha", true},
		{">0.0.0", "0.0.1-alpha", true},
		{">0", "0.0.0-alpha", true},
		{">0.0", "0.0.0-alpha", true},
		{">0.0.0", "0.0.0-alpha", true},
		{">=0", "0.0.1-alpha", true},
		{">0", "0", false},
		{">=0", "0", true},
		{"=0", "1", false},
		{">=v1.8.0", "v1.9.4-gke.1", false},         // Because the constraint does not contains prereleases, the given version won t be tested against it. For your safety.
		{">=v1.8.0-whatever", "v1.9.4-gke.1", true}, // However, that comparison will yield the expected result.

@mh-cbon
Copy link
Contributor

mh-cbon commented Apr 8, 2018

@consideRatio you might check #59

Not exactly the same matters, but its about pre-release handling when facing edge cases (note this field is generally speaking difficult to handle because it s by design loosely specified).

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

No branches or pull requests

2 participants