You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Gilbert Song <so...@gmail.com> on 2016/01/08 03:20:21 UTC
Re: Review Request 41943: Fixed stout protobuf::parse to support JSON
containing JSON::Null.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41943/
-----------------------------------------------------------
(Updated Jan. 7, 2016, 6:20 p.m.)
Review request for mesos, Ben Mahler, Artem Harutyunyan, Jie Yu, Joris Van Remoortere, Joseph Wu, and Timothy Chen.
Summary (updated)
-----------------
Fixed stout protobuf::parse to support JSON containing JSON::Null.
Bugs: MESOS-4294
https://issues.apache.org/jira/browse/MESOS-4294
Repository: mesos
Description (updated)
-------
Fixed stout protobuf::parse to support JSON containing JSON::Null.
Diffs (updated)
-----
3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 98ea47794b3a7c99b3cbd2418ba6e36eb5951259
3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp bf2a2b8a9f67c6a4cf66b156b9c14fae015a8af0
3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.h 08793c9d21d4b9b7dd3081cfa35afa47a7e0d28a
3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.cc 797b3b000f3c3fea42cabc3b40baf7235eab6b1e
3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.proto 4c11e23f875b75bcb9d5c65ee66d55b6f72e546d
Diff: https://reviews.apache.org/r/41943/diff/
Testing
-------
make check (tested on both ubuntu14.04 and OSX10.10)
Thanks,
Gilbert Song
Re: Review Request 41943: Added test case for stout protobuf parse
containing JSON null.
Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41943/#review113883
-----------------------------------------------------------
Ship it!
3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp (line 344)
<https://reviews.apache.org/r/41943/#comment174658>
How about ParseJSONNull?
3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp (line 349)
<https://reviews.apache.org/r/41943/#comment174660>
Why "message1" and "message2" for the values of 'str'? How about we set these to "value" to keep it simple?
Also, it looks like we should probably compare equality via SerializeAsString, rather than checking only one field?
3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp (line 359)
<https://reviews.apache.org/r/41943/#comment174662>
You can use the '`->`' operator rather than '`.get().`' Ditto elsewhere.
3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp (line 380)
<https://reviews.apache.org/r/41943/#comment174661>
Seems that you don't need to set the optional str here, can we omit it?
3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp (lines 386 - 387)
<https://reviews.apache.org/r/41943/#comment174659>
This can be an EXPECT and no need to store the 'parse' variable:
```
ASSERT_ERROR(protobuf::parse<tests::Nested>(json.get()););
```
- Ben Mahler
On Jan. 11, 2016, 11:40 p.m., Gilbert Song wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41943/
> -----------------------------------------------------------
>
> (Updated Jan. 11, 2016, 11:40 p.m.)
>
>
> Review request for mesos, Ben Mahler, Artem Harutyunyan, Jie Yu, Joris Van Remoortere, Joseph Wu, and Timothy Chen.
>
>
> Bugs: MESOS-4294
> https://issues.apache.org/jira/browse/MESOS-4294
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added test case for stout protobuf parse containing JSON null.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp bf2a2b8a9f67c6a4cf66b156b9c14fae015a8af0
> 3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.h 08793c9d21d4b9b7dd3081cfa35afa47a7e0d28a
> 3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.cc 797b3b000f3c3fea42cabc3b40baf7235eab6b1e
> 3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.proto 4c11e23f875b75bcb9d5c65ee66d55b6f72e546d
>
> Diff: https://reviews.apache.org/r/41943/diff/
>
>
> Testing
> -------
>
> make check (tested on both ubuntu14.04 and OSX10.10)
>
>
> Thanks,
>
> Gilbert Song
>
>
Re: Review Request 41943: Added test case for stout protobuf parse
containing JSON null.
Posted by Gilbert Song <so...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41943/
-----------------------------------------------------------
(Updated Jan. 11, 2016, 3:40 p.m.)
Review request for mesos, Ben Mahler, Artem Harutyunyan, Jie Yu, Joris Van Remoortere, Joseph Wu, and Timothy Chen.
Summary (updated)
-----------------
Added test case for stout protobuf parse containing JSON null.
Bugs: MESOS-4294
https://issues.apache.org/jira/browse/MESOS-4294
Repository: mesos
Description (updated)
-------
Added test case for stout protobuf parse containing JSON null.
Diffs
-----
3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp bf2a2b8a9f67c6a4cf66b156b9c14fae015a8af0
3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.h 08793c9d21d4b9b7dd3081cfa35afa47a7e0d28a
3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.cc 797b3b000f3c3fea42cabc3b40baf7235eab6b1e
3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.proto 4c11e23f875b75bcb9d5c65ee66d55b6f72e546d
Diff: https://reviews.apache.org/r/41943/diff/
Testing
-------
make check (tested on both ubuntu14.04 and OSX10.10)
Thanks,
Gilbert Song
Re: Review Request 41943: Fixed stout protobuf::parse to support JSON
containing JSON::Null.
Posted by Gilbert Song <so...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41943/
-----------------------------------------------------------
(Updated Jan. 11, 2016, 3:39 p.m.)
Review request for mesos, Ben Mahler, Artem Harutyunyan, Jie Yu, Joris Van Remoortere, Joseph Wu, and Timothy Chen.
Bugs: MESOS-4294
https://issues.apache.org/jira/browse/MESOS-4294
Repository: mesos
Description
-------
Fixed stout protobuf::parse to support JSON containing JSON::Null.
Diffs (updated)
-----
3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp bf2a2b8a9f67c6a4cf66b156b9c14fae015a8af0
3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.h 08793c9d21d4b9b7dd3081cfa35afa47a7e0d28a
3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.cc 797b3b000f3c3fea42cabc3b40baf7235eab6b1e
3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.proto 4c11e23f875b75bcb9d5c65ee66d55b6f72e546d
Diff: https://reviews.apache.org/r/41943/diff/
Testing
-------
make check (tested on both ubuntu14.04 and OSX10.10)
Thanks,
Gilbert Song