mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Marco Massenzio" <ma...@mesosphere.io>
Subject Re: Review Request 34943: Added validation to flags.
Date Tue, 02 Jun 2015 15:38:42 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34943/#review86225
-----------------------------------------------------------



3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp
<https://reviews.apache.org/r/34943/#comment138142>

    can you please add some documentation to explain what each type (and the relative @param)
is?
    (using Doxygen would be awesome :)



3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp
<https://reviews.apache.org/r/34943/#comment138141>

    How do we enforce that F is a function, and not, say, a std::string?



3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp
<https://reviews.apache.org/r/34943/#comment138143>

    too much choice, IMO - there are (if I counted them right) 8 different `add()` variants
to choose from (none of them documented :)
    
    It's virtually impossible for folks to figure out which to pick and this leads, I believe,
to the current 'code by copying' approach.
    
    I would cull it down to no more than 2-3 different options, with some sensible default
values.



3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp
<https://reviews.apache.org/r/34943/#comment138144>

    so, with your approach, everyone, every time that they add a required flag, will have
to, essentially, copy & paste this code.
    
    what I had envisioned, was a simpler way (eg, by adding a `bool required = false` arg)
that, if true would automatically do this validation step in `FlagsBase`
    
    (of course, we could add another `add()` method that has that signature, that just internally
implements this and calls one of the other `add()` ones - making it the ninth option :) )



3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp
<https://reviews.apache.org/r/34943/#comment138146>

    I'll admit I'm still a bith hazy about the new { } initializer, but could have this been:
    ```
    char* argv[] = { (char*) "/path/to/program",
                     (char*) "--name1=billy joel" };
    ```


- Marco Massenzio


On June 2, 2015, 2:43 p.m., Benjamin Hindman wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34943/
> -----------------------------------------------------------
> 
> (Updated June 2, 2015, 2:43 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> See summary.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/flags/flag.hpp 87606d860dea3235ec70d127d3379065d42dc90b

>   3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp 61a405f225d14acbc38a80d35570426cb05a3d0a

>   3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp a6e8ba943d97ae908122a444332155ebc6c7bb93

> 
> Diff: https://reviews.apache.org/r/34943/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>


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