mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Mahler <bmah...@apache.org>
Subject Re: Review Request 58707: Enhanced stout's Version to support prerelease and build labels.
Date Wed, 26 Apr 2017 23:16:11 GMT

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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message