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 Tue, 02 May 2017 21:13:21 GMT

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


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