> On April 26, 2017, 11:16 p.m., Benjamin Mahler wrote: > > 3rdparty/stout/include/stout/version.hpp > > Lines 51-54 (original), 60-63 (patched) > > <https://reviews.apache.org/r/58707/diff/2/?file=1699828#file1699828line64> > > > > 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), "."); > > ```
It is `raw` in the sense that it hasn't been parsed yet -- i.e., it is a string that is supposed to contain more deeply nested structure (dot-separated components, each of which is a number), but that nested structure hasn't been validated yet. I wasn't crazy about either `numericComponent` or `numericVersion`, because (a) it doesn't capture the not-parsed-yet nature of the variable (b) it is a string, it isn't (yet) numeric, (c) it contains multiple version numbers/components. I don't like omitting the variable -- IMO the logic is easier to grasp if we use a separate variable. How about `rawNumericComponents`? > On April 26, 2017, 11:16 p.m., Benjamin Mahler wrote: > > 3rdparty/stout/include/stout/version.hpp > > Lines 156-157 (patched) > > <https://reviews.apache.org/r/58707/diff/2/?file=1699828#file1699828line160> > > > > 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 The annoying thing with moving the single quote to the next line is that you can't easily `+` two character literals together, so you end up with ``` return Error("Identifier contains illegal character: " + string("'") + stringify(*firstInvalid) + "'"); ``` which IMO is harder to read overall. - Neil ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/58707/#review172994 ----------------------------------------------------------- On May 1, 2017, 11:45 p.m., Neil Conway wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/58707/ > ----------------------------------------------------------- > > (Updated May 1, 2017, 11:45 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/4/ > > > Testing > ------- > > `make check` > > > Thanks, > > Neil Conway > >
