You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Neil Conway <ne...@gmail.com> on 2017/04/26 17:35:25 UTC
Re: Review Request 58707: Enhanced stout's Version to support
prerelease and build labels.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58707/
-----------------------------------------------------------
(Updated April 26, 2017, 5:35 p.m.)
Review request for mesos, Benjamin Mahler and Kapil Arya.
Changes
-------
Tweaked comments, rebase.
Summary (updated)
-----------------
Enhanced stout's Version to support prerelease and build labels.
Bugs: MESOS-1987
https://issues.apache.org/jira/browse/MESOS-1987
Repository: mesos
Description
-------
Previously, Stout's `Version` abstraction only supported a subset of
Semver: version numbers with three numeric components (an optional
trailing "label" with a leading hyphen was supported but ignored).
This commit adds support for SemVer 2.0.0, which defines two additional
optional fields: a "prerelease label" and a "build metadata label",
e.g., "1.2.3-alpha.1+foo". Both labels consist of a series of
dot-separated identifiers.
Diffs (updated)
-----
3rdparty/stout/include/stout/version.hpp 7717c85b95d29cefe8f19f3cada4b7402d4d446f
3rdparty/stout/tests/version_tests.cpp 724ed2292fdd3c5f4c98facf82260078b66a0e97
Diff: https://reviews.apache.org/r/58707/diff/3/
Changes: https://reviews.apache.org/r/58707/diff/2-3/
Testing
-------
`make check`
Thanks,
Neil Conway
Re: Review Request 58707: Enhanced stout's Version to support
prerelease and build labels.
Posted by Benjamin Mahler <bm...@apache.org>.
> On April 26, 2017, 11:16 p.m., Benjamin Mahler wrote:
> > 3rdparty/stout/include/stout/version.hpp
> > Lines 156-157 (patched)
> > <https://reviews.apache.org/r/58707/diff/2/?file=1699828#file1699828line160>
> >
> > In general we try to open and close the quote on the same line if possible, as it tends to reduce the amount of times we forget to close the quote, ditto elsewhere
>
> Neil Conway wrote:
> The annoying thing with moving the single quote to the next line is that you can't easily `+` two character literals together, so you end up with
>
> ```
> return Error("Identifier contains illegal character: " +
> string("'") + stringify(*firstInvalid) + "'");
> ```
>
> which IMO is harder to read overall.
Per offline discussion, string literals can be concatenated without the + operator:
```
return Error("Identifier contains illegal character: "
"'" + stringify(*firstInvalid) + "'");
```
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58707/#review172994
-----------------------------------------------------------
On May 2, 2017, 8 p.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58707/
> -----------------------------------------------------------
>
> (Updated May 2, 2017, 8 p.m.)
>
>
> Review request for mesos, Benjamin Mahler and Kapil Arya.
>
>
> Bugs: MESOS-1987
> https://issues.apache.org/jira/browse/MESOS-1987
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Previously, Stout's `Version` abstraction only supported a subset of
> Semver: version numbers with three numeric components (an optional
> trailing "label" with a leading hyphen was supported but ignored).
>
> This commit adds support for SemVer 2.0.0, which defines two additional
> optional fields: a "prerelease label" and a "build metadata label",
> e.g., "1.2.3-alpha.1+foo". Both labels consist of a series of
> dot-separated identifiers.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/version.hpp 7717c85b95d29cefe8f19f3cada4b7402d4d446f
> 3rdparty/stout/tests/version_tests.cpp 925f3833fd8b1732c35663df3e63c161261e9bff
>
>
> Diff: https://reviews.apache.org/r/58707/diff/5/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 58707: Enhanced stout's Version to support
prerelease and build labels.
Posted by Neil Conway <ne...@gmail.com>.
> On April 26, 2017, 11:16 p.m., Benjamin Mahler wrote:
> > 3rdparty/stout/include/stout/version.hpp
> > Lines 51-54 (original), 60-63 (patched)
> > <https://reviews.apache.org/r/58707/diff/2/?file=1699828#file1699828line64>
> >
> > I wasn't able to see how "raw" describes what this is storing, you could call it `numericComponent` or `numericVersion`, but we could also avoid it:
> >
> > ```
> > std::vector<std::string> numeric_components =
> > strings::split(s.substr(0, end_of_numeric_component), ".");
> > ```
It is `raw` in the sense that it hasn't been parsed yet -- i.e., it is a string that is supposed to contain more deeply nested structure (dot-separated components, each of which is a number), but that nested structure hasn't been validated yet.
I wasn't crazy about either `numericComponent` or `numericVersion`, because (a) it doesn't capture the not-parsed-yet nature of the variable (b) it is a string, it isn't (yet) numeric, (c) it contains multiple version numbers/components.
I don't like omitting the variable -- IMO the logic is easier to grasp if we use a separate variable.
How about `rawNumericComponents`?
> On April 26, 2017, 11:16 p.m., Benjamin Mahler wrote:
> > 3rdparty/stout/include/stout/version.hpp
> > Lines 156-157 (patched)
> > <https://reviews.apache.org/r/58707/diff/2/?file=1699828#file1699828line160>
> >
> > In general we try to open and close the quote on the same line if possible, as it tends to reduce the amount of times we forget to close the quote, ditto elsewhere
The annoying thing with moving the single quote to the next line is that you can't easily `+` two character literals together, so you end up with
```
return Error("Identifier contains illegal character: " +
string("'") + stringify(*firstInvalid) + "'");
```
which IMO is harder to read overall.
- Neil
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58707/#review172994
-----------------------------------------------------------
On May 1, 2017, 11:45 p.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58707/
> -----------------------------------------------------------
>
> (Updated May 1, 2017, 11:45 p.m.)
>
>
> Review request for mesos, Benjamin Mahler and Kapil Arya.
>
>
> Bugs: MESOS-1987
> https://issues.apache.org/jira/browse/MESOS-1987
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Previously, Stout's `Version` abstraction only supported a subset of
> Semver: version numbers with three numeric components (an optional
> trailing "label" with a leading hyphen was supported but ignored).
>
> This commit adds support for SemVer 2.0.0, which defines two additional
> optional fields: a "prerelease label" and a "build metadata label",
> e.g., "1.2.3-alpha.1+foo". Both labels consist of a series of
> dot-separated identifiers.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/version.hpp 7717c85b95d29cefe8f19f3cada4b7402d4d446f
> 3rdparty/stout/tests/version_tests.cpp 925f3833fd8b1732c35663df3e63c161261e9bff
>
>
> Diff: https://reviews.apache.org/r/58707/diff/4/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 58707: Enhanced stout's Version to support
prerelease and build labels.
Posted by Neil Conway <ne...@gmail.com>.
> On April 26, 2017, 11:16 p.m., Benjamin Mahler wrote:
> > 3rdparty/stout/include/stout/version.hpp
> > Lines 66-71 (original), 139-182 (patched)
> > <https://reviews.apache.org/r/58707/diff/2/?file=1699828#file1699828line143>
> >
> > These are made public but I can't see how these would be useful to any callers, could we just inline lambdas them in the parse function for now until these prove needed by some callers?
I marked these as `private`, but I didn't make them lambdas yet: personally, I find large function bodies containing multiple nested lambdas hard to read (it also makes me think through whether the lambda captures any variables, etc.). Happy to discuss further if you'd prefer lambdas here.
- Neil
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58707/#review172994
-----------------------------------------------------------
On April 26, 2017, 5:35 p.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58707/
> -----------------------------------------------------------
>
> (Updated April 26, 2017, 5:35 p.m.)
>
>
> Review request for mesos, Benjamin Mahler and Kapil Arya.
>
>
> Bugs: MESOS-1987
> https://issues.apache.org/jira/browse/MESOS-1987
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Previously, Stout's `Version` abstraction only supported a subset of
> Semver: version numbers with three numeric components (an optional
> trailing "label" with a leading hyphen was supported but ignored).
>
> This commit adds support for SemVer 2.0.0, which defines two additional
> optional fields: a "prerelease label" and a "build metadata label",
> e.g., "1.2.3-alpha.1+foo". Both labels consist of a series of
> dot-separated identifiers.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/version.hpp 7717c85b95d29cefe8f19f3cada4b7402d4d446f
> 3rdparty/stout/tests/version_tests.cpp 724ed2292fdd3c5f4c98facf82260078b66a0e97
>
>
> Diff: https://reviews.apache.org/r/58707/diff/3/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 58707: Enhanced stout's Version to support
prerelease and build labels.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58707/#review172994
-----------------------------------------------------------
Really happy to see these patches!
At a high level the main thing was whether we can simplify the parsing code, by using a right-to-left parse instead of left-to-right, which seems better suited to the trailing optional prerelease version / build metadata.
3rdparty/stout/include/stout/version.hpp
Lines 39-40 (original), 41-42 (patched)
<https://reviews.apache.org/r/58707/#comment246079>
See below about documenting that this is not strictly semver, in that we allow minor and patch to be optional.
3rdparty/stout/include/stout/version.hpp
Lines 39-41 (original), 41-44 (patched)
<https://reviews.apache.org/r/58707/#comment246080>
See below about documenting that the missing minor and patch versions is technically invalid semver, maybe add a TODO to allow that only in a distinct "tolerant" parse or something.
3rdparty/stout/include/stout/version.hpp
Line 50 (original), 59 (patched)
<https://reviews.apache.org/r/58707/#comment246193>
Hm.. separator sounds like the separator itself? E.g. ".", "+", "-", etc.
Perhaps something like `end_of_numeric_component`? Or consistently using "offset" as done below?
3rdparty/stout/include/stout/version.hpp
Lines 51-54 (original), 60-63 (patched)
<https://reviews.apache.org/r/58707/#comment246192>
I wasn't able to see how "raw" describes what this is storing, you could call it `numericComponent` or `numericVersion`, but we could also avoid it:
```
std::vector<std::string> numeric_components =
strings::split(s.substr(0, end_of_numeric_component), ".");
```
3rdparty/stout/include/stout/version.hpp
Lines 61-65 (original), 73-80 (patched)
<https://reviews.apache.org/r/58707/#comment246189>
Per (2) in the spec, we have to consider any leading zeros to be invalid, and we need to reject negatives.
For the latter, I think `numify<unsigned int>(...)` would fail for negatives?
3rdparty/stout/include/stout/version.hpp
Lines 85-130 (patched)
<https://reviews.apache.org/r/58707/#comment246195>
Hm.. this was rather tricky to read, specifically handling the optionalities of prerelease version and build metadata. I wonder if it would be more readable if we were to parse from right-to-left rather than left-to-right:
In pseudo-code:
```
// Parse build metadata.
remaining, optional build_metadata = split(input, "+");
// Parse pre-release version (note that '-' is allowed within it).
remaining, optional prerelease_version = split(input, "-", 2);
// Parse numeric version.
major, optional minor, optional patch = split(remaining, ".");
```
With a right-to-left approach we could parse by splitting things out one by one, without needing to deal with the complication of dealing with whether we find the prerelease version or build metadata first / whether build metadata follows the prerelease version.
It seems to have less cognitive overhead for the reader, compared to figuring out whether the find / substr logic has any off by one errors.
Thoughts?
3rdparty/stout/include/stout/version.hpp
Lines 86 (patched)
<https://reviews.apache.org/r/58707/#comment246196>
s/=/-/
3rdparty/stout/include/stout/version.hpp
Lines 66-71 (original), 139-182 (patched)
<https://reviews.apache.org/r/58707/#comment246187>
These are made public but I can't see how these would be useful to any callers, could we just inline lambdas them in the parse function for now until these prove needed by some callers?
3rdparty/stout/include/stout/version.hpp
Lines 142 (patched)
<https://reviews.apache.org/r/58707/#comment246202>
One extra complication here is: "Numeric identifiers MUST NOT include leading zeroes" only for prerelease version. It seems ambiguous to me, for example:
1.1.1-00 // Invalid
1.1.1-00a // Valid or Invalid? Not sure if 001a is defined as numeric or not.
I think it's valid, given what is said about precedence: "identifiers consisting of only digits are compared numerically and identifiers with letters or hyphens are compared lexically in ASCII sort order". Seems to imply mixing means it's not treated as numeric validation-wise.
3rdparty/stout/include/stout/version.hpp
Lines 156-157 (patched)
<https://reviews.apache.org/r/58707/#comment246201>
In general we try to open and close the quote on the same line if possible, as it tends to reduce the amount of times we forget to close the quote, ditto elsewhere
3rdparty/stout/include/stout/version.hpp
Lines 187-188 (patched)
<https://reviews.apache.org/r/58707/#comment246184>
Ditto w.r.t. use of vector instead of string here, it seems a little confusing for the caller to have to pass vectors here instead of just taking strings IMO.
3rdparty/stout/include/stout/version.hpp
Lines 78-83 (original), 195-202 (patched)
<https://reviews.apache.org/r/58707/#comment246205>
Per my other comment, it seems this should be equivalent to:
```
!(*this < other) && !(other < *this)
```
Or just equality without the build metadata.
Looking at other libraries, they all seem to ignore build metadata for equality.
3rdparty/stout/include/stout/version.hpp
Lines 78-83 (original), 195-202 (patched)
<https://reviews.apache.org/r/58707/#comment246206>
Per my other comment, it seems this should be equivalent to:
```
!(*this < other) && !(other < *this)
```
Looking at other libraries, they all seem to ignore build metadata for equality.
3rdparty/stout/include/stout/version.hpp
Lines 78-82 (original), 195-201 (patched)
<https://reviews.apache.org/r/58707/#comment246207>
Per my other comment, it seems this should be equivalent to:
```
!(*this < other) && !(other < *this)
```
Or just equality without the build metadata.
Looking at other libraries, they all seem to ignore build metadata for equality.
3rdparty/stout/include/stout/version.hpp
Lines 80-82 (original), 197-201 (patched)
<https://reviews.apache.org/r/58707/#comment246210>
Per my comment below, I think we should have this be equivalent to:
```
!(*this < other || other < *this)
```
3rdparty/stout/include/stout/version.hpp
Lines 233-235 (patched)
<https://reviews.apache.org/r/58707/#comment246204>
Hm.. it seems to me that we should consider them equal semantically within ==, given it's not factored into precedence that seems to imply that:
1.0.1+build1
1.0.1+build2
Looking at the 3 libraries I've been comparing with, they all seem to ignore build metadata for equality.
3rdparty/stout/include/stout/version.hpp
Lines 252-256 (patched)
<https://reviews.apache.org/r/58707/#comment246211>
We try to avoid treating integers as booleans. Could you return false for these?
Ditto for the returns below.
3rdparty/stout/include/stout/version.hpp
Lines 322-323 (patched)
<https://reviews.apache.org/r/58707/#comment246183>
I was surprised that these are exposed to the caller as vectors, can we just simplify this a bit and just expose a single string for both the pre-release and build labels to start with?
Looking around at a few libraries, this seems to be the general approach taken:
https://github.com/blang/semver
https://github.com/jlindsey/semantic
https://github.com/thisandagain/semver
3rdparty/stout/tests/version_tests.cpp
Lines 53 (patched)
<https://reviews.apache.org/r/58707/#comment246055>
It seems a little odd to me that there's a separate test for this, seems like the 0.10.4 and 0.20.3 could have been integrated into the table in this test, and we just test the table is ordered more comprehensively.
3rdparty/stout/tests/version_tests.cpp
Lines 74 (patched)
<https://reviews.apache.org/r/58707/#comment246050>
Needs to be an ASSERT_SOME?
3rdparty/stout/tests/version_tests.cpp
Lines 79-85 (patched)
<https://reviews.apache.org/r/58707/#comment246049>
Since we're looping over a table we'll not know which case is failing if these EXPECTs fail and show their line number. I suspect that's why you log the input in the cases below already, mind doing that here as well?
3rdparty/stout/tests/version_tests.cpp
Lines 79-80 (patched)
<https://reviews.apache.org/r/58707/#comment246054>
Maybe a little comment:
// Check that the table is ordered.
3rdparty/stout/tests/version_tests.cpp
Lines 61-67 (original), 98-113 (patched)
<https://reviews.apache.org/r/58707/#comment246059>
It would be nice to avoid needing the output, and just expecting it to equal the input, see my question about the "1" and "1.20" cases below.
3rdparty/stout/tests/version_tests.cpp
Line 66 (original), 101 (patched)
<https://reviews.apache.org/r/58707/#comment246058>
hm.. so it looks like "1" or "1.2" are not valid semver but we allow them (regardless of your change), I see that some libraries refused to and some offer special support for it:
The most popular go library offers a tolerant parse: https://github.com/blang/semver/blob/4a1e882c79dcf4ec00d2e29fac74b9c8938d5052/semver.go#L203-L207
discussion here: https://github.com/blang/semver/issues/16
We should probably document this particular corner, and later, perhaps support it in a distinct way to clarify that it's not semver.
3rdparty/stout/tests/version_tests.cpp
Lines 80-81 (original), 126-127 (patched)
<https://reviews.apache.org/r/58707/#comment246212>
Whoops? Do you want to move this to your previous patch?
3rdparty/stout/tests/version_tests.cpp
Lines 89-96 (original), 135-157 (patched)
<https://reviews.apache.org/r/58707/#comment246188>
I think we need some leading zero cases here? Both in major/minor/patch, and in the pre-release label, see (2) and (9) in the spec.
Also, per (2), need some cases to test negatives in major, minor, or patch.
- Benjamin Mahler
On April 26, 2017, 5:35 p.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58707/
> -----------------------------------------------------------
>
> (Updated April 26, 2017, 5:35 p.m.)
>
>
> Review request for mesos, Benjamin Mahler and Kapil Arya.
>
>
> Bugs: MESOS-1987
> https://issues.apache.org/jira/browse/MESOS-1987
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Previously, Stout's `Version` abstraction only supported a subset of
> Semver: version numbers with three numeric components (an optional
> trailing "label" with a leading hyphen was supported but ignored).
>
> This commit adds support for SemVer 2.0.0, which defines two additional
> optional fields: a "prerelease label" and a "build metadata label",
> e.g., "1.2.3-alpha.1+foo". Both labels consist of a series of
> dot-separated identifiers.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/version.hpp 7717c85b95d29cefe8f19f3cada4b7402d4d446f
> 3rdparty/stout/tests/version_tests.cpp 724ed2292fdd3c5f4c98facf82260078b66a0e97
>
>
> Diff: https://reviews.apache.org/r/58707/diff/3/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 58707: Enhanced stout's Version to support
prerelease and build labels.
Posted by Neil Conway <ne...@gmail.com>.
> On May 2, 2017, 9:13 p.m., Benjamin Mahler wrote:
> > 3rdparty/stout/include/stout/version.hpp
> > Lines 102 (patched)
> > <https://reviews.apache.org/r/58707/diff/5/?file=1706035#file1706035line107>
> >
> > I guess this breaks for the negative case? E.g. 1.0.0--1.-1
> >
> > We'll treat the -1's as large numbers based on the numify test case you showed earlier? Rather than being invalid (no negative allowed) or non-numeric (due to the hyphen). Not clear which one is correct from the spec AFAICT, unfortunately :(
Per offline discussion (and https://github.com/mojombo/semver/issues/324), prerelease identifiers that begin with hyphens must be supported but should be treated as non-numeric.
> On May 2, 2017, 9:13 p.m., Benjamin Mahler wrote:
> > 3rdparty/stout/include/stout/version.hpp
> > Lines 243-264 (patched)
> > <https://reviews.apache.org/r/58707/diff/5/?file=1706035#file1706035line248>
> >
> > Should we keep the notion of "other" here rather than "1" and "2"?
> >
> > I found the nesting here to make this a little hard to follow, maybe we can use a flat list of cases to make this easier to read?
> >
> > ```
> > if (identifier.isSome() && other_identifier.isSome()) {
> > // Both are numeric.
> > if (identifier.get() != other_identifier.get()) {
> > return identifier.get() < other_identifier.get();
> > }
> > } else if (identifier.isSome()) {
> > // If `this` identifier is numeric but `other` is not, `this < other`.
> > return true;
> > } else if (other_identifier.isSome()) {
> > // If `other` identifier is numeric but `this` is not, `other < this`.
> > return false;
> > } else {
> > // Neither identifier is numeric, so compare them via ASCII sort.
> > if (prerelease.at(i) != other.prerelease.at(i)) {
> > return prerelease.at(i) < other.prerelease.at(i);
> > }
> > }
> > ```
Thanks, that is a nice cleanup.
- Neil
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58707/#review173649
-----------------------------------------------------------
On May 3, 2017, 5:30 p.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58707/
> -----------------------------------------------------------
>
> (Updated May 3, 2017, 5:30 p.m.)
>
>
> Review request for mesos, Benjamin Mahler and Kapil Arya.
>
>
> Bugs: MESOS-1987
> https://issues.apache.org/jira/browse/MESOS-1987
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Previously, Stout's `Version` abstraction only supported a subset of
> Semver: version numbers with three numeric components (an optional
> trailing "label" with a leading hyphen was supported but ignored).
>
> This commit adds support for SemVer 2.0.0, which defines two additional
> optional fields: a "prerelease label" and a "build metadata label",
> e.g., "1.2.3-alpha.1+foo". Both labels consist of a series of
> dot-separated identifiers.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/version.hpp 7717c85b95d29cefe8f19f3cada4b7402d4d446f
> 3rdparty/stout/tests/version_tests.cpp 925f3833fd8b1732c35663df3e63c161261e9bff
>
>
> Diff: https://reviews.apache.org/r/58707/diff/6/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 58707: Enhanced stout's Version to support
prerelease and build labels.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58707/#review173649
-----------------------------------------------------------
Fix it, then Ship it!
3rdparty/stout/include/stout/version.hpp
Lines 77 (patched)
<https://reviews.apache.org/r/58707/#comment246654>
See below?
3rdparty/stout/include/stout/version.hpp
Lines 102 (patched)
<https://reviews.apache.org/r/58707/#comment246658>
I guess this breaks for the negative case? E.g. 1.0.0--1.-1
We'll treat the -1's as large numbers based on the numify test case you showed earlier? Rather than being invalid (no negative allowed) or non-numeric (due to the hyphen). Not clear which one is correct from the spec AFAICT, unfortunately :(
3rdparty/stout/include/stout/version.hpp
Lines 104-111 (patched)
<https://reviews.apache.org/r/58707/#comment246669>
Did you consider making this an optional validation of validateIdentifier? That would allow us to do invariant CHECKs in the constructor, see my other comment.
3rdparty/stout/include/stout/version.hpp
Line 62 (original), 131 (patched)
<https://reviews.apache.org/r/58707/#comment246662>
Do we need a TODO here to reject negatives? Or do you want to just implement that now?
3rdparty/stout/include/stout/version.hpp
Line 66 (original), 137-144 (patched)
<https://reviews.apache.org/r/58707/#comment246668>
I guess we could do this in a separate patch, to make it clearer that we're making a change to the existing behavior rather than only adding build and prerelease label support?
3rdparty/stout/include/stout/version.hpp
Lines 73-76 (original), 156-167 (patched)
<https://reviews.apache.org/r/58707/#comment246667>
It would be nice to catch errors early here and do the CHECKs in the constructor in case someone tries to construct a malformed version:
```
{
foreach (const std::string& identifier, prerelease) {
CHECK_NONE(validateIdentifier(identifier, true)); // disallow_leading_zeros=true
}
foreach (const std::string& identifier, build) {
CHECK_NONE(validateIdentifier(identifier));
}
}
```
Separately from your change, it would be nice to also use unsigned integers to avoid needing to validate against negative numbers, but we can do this in a separate patch.
3rdparty/stout/include/stout/version.hpp
Lines 243-264 (patched)
<https://reviews.apache.org/r/58707/#comment246666>
Should we keep the notion of "other" here rather than "1" and "2"?
I found the nesting here to make this a little hard to follow, maybe we can use a flat list of cases to make this easier to read?
```
if (identifier.isSome() && other_identifier.isSome()) {
// Both are numeric.
if (identifier.get() != other_identifier.get()) {
return identifier.get() < other_identifier.get();
}
} else if (identifier.isSome()) {
// If `this` identifier is numeric but `other` is not, `this < other`.
return true;
} else if (other_identifier.isSome()) {
// If `other` identifier is numeric but `this` is not, `other < this`.
return false;
} else {
// Neither identifier is numeric, so compare them via ASCII sort.
if (prerelease.at(i) != other.prerelease.at(i)) {
return prerelease.at(i) < other.prerelease.at(i);
}
}
```
3rdparty/stout/tests/version_tests.cpp
Lines 90-91 (original), 149-152 (patched)
<https://reviews.apache.org/r/58707/#comment246670>
Oh.. I see that negatives are being caught correctly, I didn't realize when reading the code that we were catching them due to the pre-release label parsing.
Hm.. maybe we need a little note about that in the major,minor,patch numification loop?
- Benjamin Mahler
On May 2, 2017, 8 p.m., Neil Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58707/
> -----------------------------------------------------------
>
> (Updated May 2, 2017, 8 p.m.)
>
>
> Review request for mesos, Benjamin Mahler and Kapil Arya.
>
>
> Bugs: MESOS-1987
> https://issues.apache.org/jira/browse/MESOS-1987
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Previously, Stout's `Version` abstraction only supported a subset of
> Semver: version numbers with three numeric components (an optional
> trailing "label" with a leading hyphen was supported but ignored).
>
> This commit adds support for SemVer 2.0.0, which defines two additional
> optional fields: a "prerelease label" and a "build metadata label",
> e.g., "1.2.3-alpha.1+foo". Both labels consist of a series of
> dot-separated identifiers.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/version.hpp 7717c85b95d29cefe8f19f3cada4b7402d4d446f
> 3rdparty/stout/tests/version_tests.cpp 925f3833fd8b1732c35663df3e63c161261e9bff
>
>
> Diff: https://reviews.apache.org/r/58707/diff/5/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Neil Conway
>
>
Re: Review Request 58707: Enhanced stout's Version to support
prerelease and build labels.
Posted by Neil Conway <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58707/
-----------------------------------------------------------
(Updated May 3, 2017, 6:23 p.m.)
Review request for mesos, Benjamin Mahler and Kapil Arya.
Changes
-------
Change handling of prerelease identifiers that start with hyphens.
Bugs: MESOS-1987
https://issues.apache.org/jira/browse/MESOS-1987
Repository: mesos
Description
-------
Previously, Stout's `Version` abstraction only supported a subset of
Semver: version numbers with three numeric components (an optional
trailing "label" with a leading hyphen was supported but ignored).
This commit adds support for SemVer 2.0.0, which defines two additional
optional fields: a "prerelease label" and a "build metadata label",
e.g., "1.2.3-alpha.1+foo". Both labels consist of a series of
dot-separated identifiers.
Diffs (updated)
-----
3rdparty/stout/include/stout/version.hpp 7717c85b95d29cefe8f19f3cada4b7402d4d446f
3rdparty/stout/tests/version_tests.cpp 925f3833fd8b1732c35663df3e63c161261e9bff
Diff: https://reviews.apache.org/r/58707/diff/7/
Changes: https://reviews.apache.org/r/58707/diff/6-7/
Testing
-------
`make check`
Thanks,
Neil Conway
Re: Review Request 58707: Enhanced stout's Version to support
prerelease and build labels.
Posted by Neil Conway <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58707/
-----------------------------------------------------------
(Updated May 3, 2017, 5:30 p.m.)
Review request for mesos, Benjamin Mahler and Kapil Arya.
Changes
-------
Address review comments.
Bugs: MESOS-1987
https://issues.apache.org/jira/browse/MESOS-1987
Repository: mesos
Description
-------
Previously, Stout's `Version` abstraction only supported a subset of
Semver: version numbers with three numeric components (an optional
trailing "label" with a leading hyphen was supported but ignored).
This commit adds support for SemVer 2.0.0, which defines two additional
optional fields: a "prerelease label" and a "build metadata label",
e.g., "1.2.3-alpha.1+foo". Both labels consist of a series of
dot-separated identifiers.
Diffs (updated)
-----
3rdparty/stout/include/stout/version.hpp 7717c85b95d29cefe8f19f3cada4b7402d4d446f
3rdparty/stout/tests/version_tests.cpp 925f3833fd8b1732c35663df3e63c161261e9bff
Diff: https://reviews.apache.org/r/58707/diff/6/
Changes: https://reviews.apache.org/r/58707/diff/5-6/
Testing
-------
`make check`
Thanks,
Neil Conway
Re: Review Request 58707: Enhanced stout's Version to support
prerelease and build labels.
Posted by Neil Conway <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58707/
-----------------------------------------------------------
(Updated May 2, 2017, 8 p.m.)
Review request for mesos, Benjamin Mahler and Kapil Arya.
Changes
-------
Address review comments.
Bugs: MESOS-1987
https://issues.apache.org/jira/browse/MESOS-1987
Repository: mesos
Description
-------
Previously, Stout's `Version` abstraction only supported a subset of
Semver: version numbers with three numeric components (an optional
trailing "label" with a leading hyphen was supported but ignored).
This commit adds support for SemVer 2.0.0, which defines two additional
optional fields: a "prerelease label" and a "build metadata label",
e.g., "1.2.3-alpha.1+foo". Both labels consist of a series of
dot-separated identifiers.
Diffs (updated)
-----
3rdparty/stout/include/stout/version.hpp 7717c85b95d29cefe8f19f3cada4b7402d4d446f
3rdparty/stout/tests/version_tests.cpp 925f3833fd8b1732c35663df3e63c161261e9bff
Diff: https://reviews.apache.org/r/58707/diff/5/
Changes: https://reviews.apache.org/r/58707/diff/4-5/
Testing
-------
`make check`
Thanks,
Neil Conway
Re: Review Request 58707: Enhanced stout's Version to support
prerelease and build labels.
Posted by Neil Conway <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58707/
-----------------------------------------------------------
(Updated May 1, 2017, 11:45 p.m.)
Review request for mesos, Benjamin Mahler and Kapil Arya.
Changes
-------
Checkpoint addressing review comments.
Bugs: MESOS-1987
https://issues.apache.org/jira/browse/MESOS-1987
Repository: mesos
Description
-------
Previously, Stout's `Version` abstraction only supported a subset of
Semver: version numbers with three numeric components (an optional
trailing "label" with a leading hyphen was supported but ignored).
This commit adds support for SemVer 2.0.0, which defines two additional
optional fields: a "prerelease label" and a "build metadata label",
e.g., "1.2.3-alpha.1+foo". Both labels consist of a series of
dot-separated identifiers.
Diffs (updated)
-----
3rdparty/stout/include/stout/version.hpp 7717c85b95d29cefe8f19f3cada4b7402d4d446f
3rdparty/stout/tests/version_tests.cpp 925f3833fd8b1732c35663df3e63c161261e9bff
Diff: https://reviews.apache.org/r/58707/diff/4/
Changes: https://reviews.apache.org/r/58707/diff/3-4/
Testing
-------
`make check`
Thanks,
Neil Conway