> On May 28, 2015, 11:45 a.m., Benjamin Hindman wrote:
> > src/cli/execute.cpp, line 321
> > <https://reviews.apache.org/r/34195/diff/3/?file=972213#file972213line321>
> >
> > I'm not convinced the 'errorMessage' is more help here. You're
> > basically saving doing a 'return EXIT_FAILURE;' in each of these if's,
> > which is not a big deal and arguably more explicit and clear. On the other
> > hand, this code was originally meant to be structured so the line that did
> > a 'flags.master.get()' was as close to the line that checked
> > 'flags.master.isSome()' and by moving that code farther away it makes it so
> > a reader has to look harder to confirm that when someone is "dereferencing"
> > 'flags.master' it's indeed safe.
As it happens, I too was only half-convinced that mine was a real improvement.
However, in the latest change, the `errorMessage` is actually a concatenation
of various errors: the benefit is that I don't have to (in theory) launch the
darn thing three or four times, each time discovering I missed yet another flag
:)
```
$ execute
Missing --master
{darn!}
$ execute --master=localhost
Missing --name
{oh, ffs...}
$ execute --master=localhost --name=my-master
Missing --command
{goddammit!}
$ execute --master=localhost --name=my-master --command=foobar
Could not parse master=localhost
{@#@#!!"£}
```
With this patch, one gets:
```
$ execute
Missing --master=IP:PORT
Missing --name=NAME
Missing --command=COMMAND
...
```
I don't know, I believe in karma... ;)
However, happy to change it back to the original version (with improvements) if
you think this is not really worth the extra effort.
> On May 28, 2015, 11:45 a.m., Benjamin Hindman wrote:
> > src/cli/execute.cpp, line 336
> > <https://reviews.apache.org/r/34195/diff/3/?file=972213#file972213line336>
> >
> > This pattern is why we use Option!
I totally get it, trust me!
However, (see comment above) concatenating Option<string>s is a major pain,
while here, there is actually the same level of indirection/etc.
> On May 28, 2015, 11:45 a.m., Benjamin Hindman wrote:
> > src/cli/resolve.cpp, line 81
> > <https://reviews.apache.org/r/34195/diff/3/?file=972214#file972214line81>
> >
> > I liked your pattern of doing 'setUsageMessage' immediately after
> > instantiating the class. Any reason not to do that here?
indeed, why not?
> On May 28, 2015, 11:45 a.m., Benjamin Hindman wrote:
> > src/cli/resolve.cpp, line 128
> > <https://reviews.apache.org/r/34195/diff/3/?file=972214#file972214line128>
> >
> > Let's also use this review as an opportunity to do 'EXIT_FAILURE'
> > anyplace we 'return -1' and 'EXIT_SUCCESS' anyplace we 'return 0'. Please
> > do a sweep in every file you've touched and make it so, thanks Marco!
oh, yes! thanks!!!
I really was worried people would yell at me, but I really was itching to do
that :)
With pleasure!
> On May 28, 2015, 11:45 a.m., Benjamin Hindman wrote:
> > src/examples/persistent_volume_framework.cpp, line 423
> > <https://reviews.apache.org/r/34195/diff/3/?file=972216#file972216line423>
> >
> > How come this 'usage' isn't getting killed?
I'm not very good with tedious tasks :)
thanks for catching me!
> On May 28, 2015, 11:45 a.m., Benjamin Hindman wrote:
> > src/log/tool/read.cpp, line 73
> > <https://reviews.apache.org/r/34195/diff/3/?file=972222#file972222line73>
> >
> > Please end sentence with period (here and everywhere please).
or, even better, actually fix the TODO!
- Marco
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34195/#review85536
-----------------------------------------------------------
On May 28, 2015, 10:06 a.m., Marco Massenzio wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34195/
> -----------------------------------------------------------
>
> (Updated May 28, 2015, 10:06 a.m.)
>
>
> Review request for mesos, Benjamin Hindman and Joris Van Remoortere.
>
>
> Bugs: MESOS-2711
> https://issues.apache.org/jira/browse/MESOS-2711
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Jira: MESOS-2711
>
> All the main() methods have been refactored to use the
> definition of FlagsBase::help flag and FlagsBase::usage().
>
> This CL also tries to bring some uniformity to the
> use of exit codes: if this is deemed to be worth
> making it uniform, we can come up with common
> rules and extend the changes here to be compliant.
>
> This touches a lot of files, but keep scrolling, and you will see a pattern
> emerge.
>
>
> Diffs
> -----
>
> src/cli/execute.cpp dbd19e67f56a150f54180ad13e6402842eb68e17
> src/cli/resolve.cpp a99b6094dffc9f7aa44bcf63ad40121e1abb120b
> src/examples/load_generator_framework.cpp
> be1a3bf5f16bd811cb4039c8f15478183712a426
> src/examples/persistent_volume_framework.cpp
> 8a893fcfa3d5d988a88fdeaf0bfc08e0a49b7a65
> src/health-check/main.cpp a4ce742ab8deff1ebd99359112670493fdaeeac3
> src/launcher/executor.cpp de6f1b104a765a8e53934154e78872b03695b24c
> src/local/main.cpp a641b9e83862743890597a2981a9419517e7c589
> src/log/tool/benchmark.cpp 01e55115f35d155efbea190b5308b294ba76e7cb
> src/log/tool/initialize.cpp ccda7fb1c0f7113f865ec61adee76b2ea6180442
> src/log/tool/read.cpp d14138502f5bc9a725deb83da505765865da017f
> src/log/tool/replica.cpp 3985fc7df4f7153ae623589fbdd769ccbae57125
> src/master/main.cpp d5666bc8ee8d7a0f0b8685f76d65dd1f9ac2a280
> src/slave/containerizer/isolators/network/port_mapping.hpp
> c72fb47f60f40cda8d84a10497b9133f83cf018e
> src/slave/containerizer/isolators/network/port_mapping.cpp
> 49e983edab598e2ac487bb488fdd12840a9e7dfc
> src/slave/main.cpp f762f5b06be74c391cbc336b2da28f8358952ba4
> src/tests/main.cpp e3fff5d60c0468c0d258f2bb301efc1309c071b0
> src/usage/main.cpp 97f55e938dc7678f8331970d8953d09218f70902
>
> Diff: https://reviews.apache.org/r/34195/diff/
>
>
> Testing
> -------
>
> make check
>
> **NOTE** this fixes completely the chained changes from 34193 and makes all
> the tests pass.
>
>
> Thanks,
>
> Marco Massenzio
>
>
