> On June 3, 2015, 6:49 p.m., Niklas Nielsen wrote:
> > src/common/http.cpp, line 209
> > <https://reviews.apache.org/r/34687/diff/5/?file=976831#file976831line209>
> >
> >     We haven't established a doxygen style in the style guide yet, but 
> > shouldn't we expand this to:
> >     
> >     /**
> >      *  Foo Bar Baz
> >      */
> >     
> >     ?
> 
> Niklas Nielsen wrote:
>     Gawk - review board didn't catch my newlines:
>     
>     ```
>     /**
>      * Foo bar baz
>      */
>     ```
>     
>     was my intent.

you can have the "brief format" for short comments


> On June 3, 2015, 6:49 p.m., Niklas Nielsen wrote:
> > src/common/parse.hpp, line 178
> > <https://reviews.apache.org/r/34687/diff/5/?file=976832#file976832line178>
> >
> >     What if in() is an error? Shouldn't we check for that too?

no - `parse()` would have resulted in an error (see L170)
it is non-intuitive, that's why I defined this code "gnarly": one has to dip 
one's hand deep in the cookie jar to retrieve what one's lookign for :)


> On June 3, 2015, 6:49 p.m., Niklas Nielsen wrote:
> > src/tests/common/http_tests.cpp, line 121
> > <https://reviews.apache.org/r/34687/diff/5/?file=976833#file976833line121>
> >
> >     Mind adding a test description?

No, I don't mind at all - but why here? it's not done anywhere else, and it's a 
pretty straightforward test.
Done.


> On June 3, 2015, 6:49 p.m., Niklas Nielsen wrote:
> > src/tests/common/http_tests.cpp, line 129
> > <https://reviews.apache.org/r/34687/diff/5/?file=976833#file976833line129>
> >
> >     Do you want ASSERT_SOME(ip.in());?

Done - but it's largely redundant: we DO know the IP address is an invariant 
(const string) and well-formed: the only way this breaks is if we introduce a 
bug in net::IP::parse() - but that should be caught by **those** unit tests, 
now, shouldn't it? ;)


> On June 3, 2015, 6:49 p.m., Niklas Nielsen wrote:
> > src/tests/common/http_tests.cpp, lines 139-144
> > <https://reviews.apache.org/r/34687/diff/5/?file=976833#file976833line139>
> >
> >     As jsonMasterInfo will be a json object, how about doing equality 
> > between json objects instead of stringifying?
> >     
> >     auto expected = JSON::parse("{ hostname ...");
> >     
> >     ASSERT_EQ(expected.get(), jsonMasterInfo.get());
> >     
> >     That would be robust to any spaces,newlines in the inlined JSON.
> >     
> >     If you like it, we can do it here and below.

done - btw, that code would not compile; but a variation thereof did, which is 
great!
(I did try that before but could not get it to compile, so eventually gave up - 
other JSON-related tests in the codebase do use `stringify` instead)


> On June 3, 2015, 6:49 p.m., Niklas Nielsen wrote:
> > src/tests/common/http_tests.cpp, line 161
> > <https://reviews.apache.org/r/34687/diff/5/?file=976833#file976833line161>
> >
> >     End with period.

hey, at least I'm consistently wrong! :)


> On June 3, 2015, 6:49 p.m., Niklas Nielsen wrote:
> > src/tests/common/http_tests.cpp, line 162
> > <https://reviews.apache.org/r/34687/diff/5/?file=976833#file976833line162>
> >
> >     Need an ASSERT_SOME() on jsonMasterInfo.get().find<JSON::String>("id") ?

I don't think so (but happy to be convinced otherwise): `id` is a required 
field, so if not present, it will cause the `model()` above to error out, and 
that will be caught by the `ASSERT_SOME(jsonMasterInfo)` above.
The only way this fails if we, due to a refactoring, "forget" to add back the 
"id" in the JSON - in which case, the breakage is A Good Thing, maybe?


> On June 3, 2015, 6:49 p.m., Niklas Nielsen wrote:
> > src/tests/common/parse_tests.cpp, line 19
> > <https://reviews.apache.org/r/34687/diff/5/?file=976834#file976834line19>
> >
> >     Shouldn't this include be located near line 35?

not according to Google (and cpplint): this is the "related header" that should 
precede every other headers (system, C++, etc.)
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Names_and_Order_of_Includes


> On June 3, 2015, 6:49 p.m., Niklas Nielsen wrote:
> > src/tests/common/parse_tests.cpp, line 47
> > <https://reviews.apache.org/r/34687/diff/5/?file=976834#file976834line47>
> >
> >     Test description :)

done (everywhere, actually).
Out of curiousity: is this a general requirement?
I haven't found anything in the style guide (Mesos and/or Google) that says 
anything about mandatory comments for TEST()s (in fact, I'm positive we did not 
required that at G).

(if so, however, it would have been nice to know during the first round ;)


> On June 3, 2015, 6:49 p.m., Niklas Nielsen wrote:
> > src/tests/common/parse_tests.cpp, lines 142-145
> > <https://reviews.apache.org/r/34687/diff/5/?file=976834#file976834line142>
> >
> >     Just tried this out: include <mesos/type_utils.hpp> and you should have 
> > the == operator overload, be able to strip the '.SerializeAsString()' and 
> > kill the comment :)

yay! thanks :)


- Marco


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


On June 2, 2015, 9:52 p.m., Marco Massenzio wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34687/
> -----------------------------------------------------------
> 
> (Updated June 2, 2015, 9:52 p.m.)
> 
> 
> Review request for mesos, haosdent huang and Niklas Nielsen.
> 
> 
> Bugs: MESOS-2340
>     https://issues.apache.org/jira/browse/MESOS-2340
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Jira: MESOS-2340
> 
> This is a preliminary step to enabling JSON API
> for Master Discovery via Zookeeper.
> 
> We currently save the MasterInfo PB to ZK
> serializing directly the binary data, so that
> for clients to retrieve that information, they
> need to either link up with libmesos or
> obtain the PB definition (in mesos/mesos.proto).
> 
> This change only provides the (de)serialization
> utility methods and associated tests.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am f2110a360b6e2a1e99e1d9630d5dfacfd818530a 
>   src/common/http.hpp afce7fea334c7bfa57efc48c413bf906a2ebde32 
>   src/common/http.cpp 2ac7fba7a3aac913540f1b09768777393b79284a 
>   src/common/parse.hpp 8d7ddd6819dad98cd96d5aaae8fe57caf1ee7098 
>   src/tests/common/http_tests.cpp f087b2313a13c3199b70b3d7feb728e1449a52e7 
>   src/tests/common/parse_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/34687/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Marco Massenzio
> 
>

Reply via email to