> 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. > > Marco Massenzio wrote: > 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.
Given that (1) there are lots of other instances where we could also do this but don't and (2) there is probably a better way to capture "required" fields let's not do it for now (I'll just pull this out for you though since I'm going to commit now). - Benjamin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34195/#review85536 ----------------------------------------------------------- On May 29, 2015, 1:10 a.m., Marco Massenzio wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/34195/ > ----------------------------------------------------------- > > (Updated May 29, 2015, 1:10 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/mesos.cpp 171a707cd2ba2348898e7fbe8fe9f0634edd6d86 > src/cli/resolve.cpp a99b6094dffc9f7aa44bcf63ad40121e1abb120b > src/docker/executor.cpp 075c6b5b787532bb1a85018b966aa7e2a1add4f5 > src/examples/load_generator_framework.cpp > be1a3bf5f16bd811cb4039c8f15478183712a426 > src/examples/low_level_scheduler_libprocess.cpp > bee2e7ef8432cc42733260c668f1100c68f73b8d > src/examples/low_level_scheduler_pthread.cpp > fb8cd66c2e94270971184c1e3dcc92eccab8b223 > 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.hpp e0109e20d354a6545ab10750e5d2c3e800f7a6a8 > src/log/tool/benchmark.cpp 01e55115f35d155efbea190b5308b294ba76e7cb > src/log/tool/initialize.hpp 10ac269627052d89e1936e891127486d88c74253 > src/log/tool/initialize.cpp ccda7fb1c0f7113f865ec61adee76b2ea6180442 > src/log/tool/read.hpp 9a6971bca7efc1ddd45efaa44733c1a474804da0 > 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 > 6579286b779882bec493a8e0f10486adc316dc6e > src/slave/containerizer/isolators/network/port_mapping.cpp > 843e52d6f9923d0ee0a0297cd5c464b8b72f5de3 > 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 > >
