----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32163/#review77287 -----------------------------------------------------------
Thanks Alex! This will be an awesome extension of our JSON library :) 3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp <https://reviews.apache.org/r/32163/#comment125185> Can you hightlight why you are making a forward declaration of this? Also, why not make this a member function of Value? We already have Value::as<>() and Value::is<>() and you wouldn't (AFAIK) have to forward declare. 3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp <https://reviews.apache.org/r/32163/#comment125188> s/contains/'contains'/ or s/contains/contains()/ 3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp <https://reviews.apache.org/r/32163/#comment125191> This could be a nested struct of Value (and good for struct hiding in general :) 3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp <https://reviews.apache.org/r/32163/#comment125190> Let's aim for full sentences; 'This is the main recursive operation.' 3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp <https://reviews.apache.org/r/32163/#comment125206> Mind add a comment here too, like you did above 3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp <https://reviews.apache.org/r/32163/#comment125194> We usually aim for full words; should be valueIterator in this case. 3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp <https://reviews.apache.org/r/32163/#comment125220> I know you got the notion of object and value from the comparator above, but find it hard to think about compared to using notions like lhs/rhs. 3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp <https://reviews.apache.org/r/32163/#comment125217> I wish we could use foreachpair(const Key& key, const Value& value, values) somehow. It took me a while to be convinced that the 'forwarding' piece worked. 3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp <https://reviews.apache.org/r/32163/#comment125211> Can you help me understand this as well? 3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp <https://reviews.apache.org/r/32163/#comment125216> We usually do error checking first. Here that would be to negate the test and return false first. It becomes more apparent with nested control flow in 'bool operator () (const Array& array) const' That is also what you are doing in 'bool operator () (const Object& object) const' :) 3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp <https://reviews.apache.org/r/32163/#comment125215> Can we move the nested control flow around so we negate the test and return earlier? 3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp <https://reviews.apache.org/r/32163/#comment125214> How about making a const reference to value.as<Array>() ? 3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp <https://reviews.apache.org/r/32163/#comment125193> Can you use foreach here? 3rdparty/libprocess/3rdparty/stout/tests/json_tests.cpp <https://reviews.apache.org/r/32163/#comment125178> If the JSON string is ill-formed, the test will break with less useful error output compared to using EXPECT_SOME_TRUE() Can you do a scan for those (EXPECT_SOME_FALSE() should also be available) 3rdparty/libprocess/3rdparty/stout/tests/json_tests.cpp <https://reviews.apache.org/r/32163/#comment125168> Mind adding two newlines between the 'sub' tests? It makes it a bit easier to read :) 3rdparty/libprocess/3rdparty/stout/tests/json_tests.cpp <https://reviews.apache.org/r/32163/#comment125180> s/'s// or s/'// 3rdparty/libprocess/3rdparty/stout/tests/json_tests.cpp <https://reviews.apache.org/r/32163/#comment125171> Seems like a bit overuse of value1, how about creating new JSON::Values so we can tell the 'sub' test cases apart? 3rdparty/libprocess/3rdparty/stout/tests/json_tests.cpp <https://reviews.apache.org/r/32163/#comment125181> s/'// - Niklas Nielsen On March 20, 2015, 6:23 a.m., Alexander Rojas wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/32163/ > ----------------------------------------------------------- > > (Updated March 20, 2015, 6:23 a.m.) > > > Review request for mesos, Benjamin Hindman, Bernd Mathiske, Joerg Schad, > Niklas Nielsen, and Till Toenshoff. > > > Bugs: MESOS-2510 > https://issues.apache.org/jira/browse/MESOS-2510 > > > Repository: mesos > > > Description > ------- > > Adds a function which allows to perform comparison tests on subsets of json > blobs. i.e. > > ```cpp > JSON::Value expected = JSON::parse( > "{" > " \"key\" : true" > "}").get(); > > // Returned json: > // { > // "uptime" : 45234.123, > // "key" : true > // } > JSON::Value actual = bar(); > > // I'm only interested on the "key" entry and ignore the rest. > EXPECT_TRUE(contains(actual, expected)); > ``` > > Increasing readability for tests that include json. > > For more information on the reason of why this patch is needed, please check > the JIRA entry. > > > Diffs > ----- > > 3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp > 334c898906018be6e663f53815abbe047806b95c > 3rdparty/libprocess/3rdparty/stout/tests/json_tests.cpp > f60d1bbe60f2e2b6460c06bba98e8b85ebb6a3f9 > > Diff: https://reviews.apache.org/r/32163/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Alexander Rojas > >
