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 Fri, 05 Jun 2015 23:56:03 GMT


> On June 2, 2015, 3:38 p.m., Marco Massenzio wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp, line 184
> > <https://reviews.apache.org/r/34943/diff/1/?file=976658#file976658line184>
> >
> >     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.
> 
> Benjamin Hindman wrote:
>     I completely agree this needs documentation, which I'll take on in a different review.
But there are really only 4 variants here:
>     
>     (1) The flag _is_ a class member.
>         (A) The flag has a default value (i.e., is _not_ an Option<T>).
>         (B) The flag does not have a default value (i.e., _is_ an Option<T>).
>     
>     (2) The flag is _not_ a class member.
>         (A) The flag has a default value (i.e., is _not_ an Option<T>).
>         (B) The flag does not have a default value (i.e., _is_ an Option<T>).
>     
>     All 4 of these options can optionally have a validation function, but since we can't
capture the default validation function with the existing 4 functions we have to do the delegating
function trick, resulting in 8 functions. Bottom line, this needs to be documented (and if
someone has a better suggestion to avoid the delgating function trick I'm all ears).

Why should the callers care if it has a default value, type-wise?

My thinking was, let's always use an `Option<T>` for the flag; a, possibly `None()`,
default value and a `required` bool flag:
```
template<typename T>
void add(Option<T>* option, 
         const std::string& name, 
         const std::string& help,
         const Option<T>& const default = None(),
         bool required = false)
```
and similarly for the class member.

So, all we have are *two* options, and no doubt as to which one to use.
I'm sure I'm missing a ton here, though... especially given that:

```
  Option<string> foo;
  Option<string> bar;

  Option<string> def_foo{"default-foo-val"};

  flags.add(&foo, "foo", "this is foo!", def_foo, true);
  flags.add1(&bar, "bar", "this is BAR");
```
works as intended, but:
```
flags.add(&foo, "foo", "this is foo!", "default-foo-val", true);
```
causes a compile error (even though, I can totally see an `Option(const T& _t)` constructor
that should "just work" (as it very much does for `def_foo`).


> On June 2, 2015, 3:38 p.m., Marco Massenzio wrote:
> > 3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp, lines 500-504
> > <https://reviews.apache.org/r/34943/diff/1/?file=976659#file976659line500>
> >
> >     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 :) )
> 
> Benjamin Hindman wrote:
>     I wasn't trying to solve the "required" flag problem, I was just trying to show an
example of a validation function. But I violently agree that this is therefore a horribly
bad example that sets a bad precedent, so I've replaced this with a different validation function.
>     
>     I also agree that we should introduce a simplier way, such as a bool as you suggested
(or better an an enumeration). This doesn't completley solve the problem, however, since a
flag without a default value will still be an Option and still require one to do 'CHECK_SOME(flags.flag)'.
Got any other ideas?

> horribly bad example
I never said that :)

As mentioned above, a flag should *always* be an `Option<T>` with the difference that,
if we had marked it as `required` then we'd be confident that, once loaded, it does have a
value (so, no need to `CHECK_SOME()`) or it would have already failed.

For the optional (aka `non-required`) ones (without a default value) obviously they could
be `None()` but that is an "expected outcome" and the caller can take whatever action they
deem appropriate.

>From the 'client' perspective:

1. I told you it was `required` --> there must be a value (or you'd have failed before
getting back to me)
2. I gave you a default value --> there must be a value (possibly the default one)
3. It's neither required, nor has a default --> its absence (`isNone() == true`) has a
meaning to me and I'll take appropriate action
4. It's required, and I gave you a default value --> I'm bad at logical thinking ;-)

I would also propose (similarly to the --help case) to add an `onMissingRequired()` "hook"
with the default implementation 
```
void onMissingRequired(const string& name)
{
  exit(EXIT_FAILURE) << usage("Missing required flag --" + name);
}
```
so that the caller can also take some alternative action.


- Marco


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


On June 5, 2015, 2:48 p.m., Benjamin Hindman wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34943/
> -----------------------------------------------------------
> 
> (Updated June 5, 2015, 2:48 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Also refactored existing 'lambda::bind' arguments to use C++11 lambdas, enabling us to
get rid of our "loader" and "stringifier" functors.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/3rdparty/stout/include/Makefile.am 6ac2f04a6a1d8db1725972a3b4b46a0dd734566f

>   3rdparty/libprocess/3rdparty/stout/include/stout/flags/flag.hpp 87606d860dea3235ec70d127d3379065d42dc90b

>   3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp ee855da6128c2d671eb2fc7eaa2c0aab2341aebb

>   3rdparty/libprocess/3rdparty/stout/include/stout/flags/loader.hpp 51d3ab020bd2bae1f12b66cac0da5383c813d5d7

>   3rdparty/libprocess/3rdparty/stout/include/stout/flags/stringifier.hpp fda5ae1328a23a4371475652f921341dce7448d5

>   3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp 80450185f60c5b273face490e0bb9e695b0cb984

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