Wasn't sure if you guys saw this? On Wed, Jun 24, 2015 at 3:22 PM, Ben Mahler <benjamin.mah...@gmail.com> wrote:
> This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/34943/ > 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp > <https://reviews.apache.org/r/34943/diff/5/?file=986809#file986809line790> > (Diff > revision 5) > > inline Try<Nothing> FlagsBase::load( > > 748 > > // Validate the flags value. > > 749 > > // > > 750 > > // TODO(benh): Consider validating all flags at the same time in > > 751 > > // order to provide more feedback rather than requiring a user to > > 752 > > // fix one at a time. > > 753 > > foreachvalue (const Flag& flag, flags_) { > > 754 > > Option<Error> error = flag.validate(*this); > > 755 > > if (error.isSome()) { > > 756 > > return error.get(); > > 757 > > } > > 758 > > } > > Seems nice to have this print the same kind of error strings as above, so > that the caller can just compose a specific error, rather than having to > explicitly include the flag name: > > foreachvalue (const Flag& flag, flags_) { > Option<Error> error = flag.validate(*this); > if (error.isSome()) { > return "Invalid value for '" + flag.name + "': " + error.get(); > } > } > > Then the caller can just do: > > add(&Flags::duration, > "duration", > "", > DEFAULT_DURATION, > [](const Duration& value) -> Option<Error> { > if (value < Seconds(1)) { > return Error("Must be greater than " + stringify(Seconds(1))); > } > return None(); > }); > > vs. > > add(&Flags::duration, > "duration", > "", > DEFAULT_DURATION, > [](const Duration& value) -> Option<Error> { > if (value < Seconds(1)) { > return Error("--duration must be greater than " + > stringify(Seconds(1))); > } > return None(); > }); > > Thoughts? > > Thoughts? > > > - Ben Mahler > > On June 18th, 2015, 3:34 p.m. UTC, Benjamin Hindman wrote: > Review request for mesos and Niklas Nielsen. > By Benjamin Hindman. > > *Updated June 18, 2015, 3:34 p.m.* > *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. > > Also, followed up as requested of some reviewers and added initial > documentation for flags in https://reviews.apache.org/r/35611. > > Testing > > make check > > 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) > > View Diff <https://reviews.apache.org/r/34943/diff/> >
