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 Tue, 02 May 2017 20:00:12 GMT


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


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