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