mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Neil Conway <neil.con...@gmail.com>
Subject Re: Review Request 58707: Enhanced stout's Version to support prerelease and build labels.
Date Wed, 03 May 2017 18:22:29 GMT


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


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