----------------------------------------------------------- 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 > >
