CXX-2096 Add BSON UDL for from_json() #724
Conversation
Codecov Report
@@ Coverage Diff @@
## master #724 +/- ##
==========================================
- Coverage 94.31% 92.65% -1.66%
==========================================
Files 357 357
Lines 18663 20052 +1389
==========================================
+ Hits 17602 18580 +978
- Misses 1061 1472 +411
Continue to review full report at Codecov.
|
samantharitter
left a comment
samantharitter
left a comment
There was a problem hiding this comment.
These code changes look pretty good, just a few comments!
But: I have some questions about whether this is a useful thing to add to the driver. Did a request for this feature come from the community? Maybe @bazile-clyde knows?
Since this option can't take a JSON representation mode, it's a little less useful than the to_json() method without really being much quicker or shorter to type. Also I don't think the UDL syntax is super widely known, so I'm not sure people would use this unless it was a specific community-requested feature. I'm not sure it's actually worth us committing and then supporting this.
src/bsoncxx/json.cpp
Outdated
| } | ||
|
|
||
| document::value BSONCXX_CALL operator"" _bson(const char* str, size_t len) { | ||
| // len isn't used, but it is required for the UDL to use a char* |
There was a problem hiding this comment.
I think you can get around this by changing the signature so it does not name the len variable:
document::value BSONCXX_CALL operator"" _bson(const char* str, size_t) {...}
But also, it looks like there should be a signature possible that just takes (const char*) according to this page, did you have trouble implementing that?
There was a problem hiding this comment.
I'll try it with document::value BSONCXX_CALL operator"" _bson(const char* str, size_t) {...}! Thanks for letting me know you could do that.
I originally tried just using (const char*) but it's not an applicable literal operator. On that page, there is a list of accepted parameters and it lists (const char*) as being only used:
as fallbacks for integer and floating-point user-defined literals
src/bsoncxx/json.hpp
Outdated
| BSONCXX_API document::value BSONCXX_CALL from_json(stdx::string_view json); | ||
|
|
||
| /// | ||
| /// Constructs a new document::value from the provided JSON text |
There was a problem hiding this comment.
Nit, please add a period at the end of the sentence
| R"({ "number" : { "$numberInt" : "42" }, "bin" : { "$binary" : { "base64": "ZGVhZGJlZWY=", "subType" : "04" } } })"); | ||
| } | ||
|
|
||
| TEST_CASE("UDL _bson works like from_json()") { |
There was a problem hiding this comment.
For completeness' sake, please add a test for the empty document "{}"
@samantharitter This came out of the usability requests from the engineers that worked on genny as a nice to have. There are also a few tickets that point to nlohmann/JSON as a model for easily creating documents. One of the simpler ways to generate JSON in that library is also a UDL. As far as how widely known UDLs are, I'm not sure. Strings, dates, numeric types, etc. in the standard library all have them. So they aren't hidden. Is it worth committing and supporting? I think so. It's just a wrapper function. So about 3 lines of code for quick usability improvement. With all of the complaints we've received about how difficult it is to create BSON documents, I don't see why not. |
samantharitter
left a comment
samantharitter
left a comment
There was a problem hiding this comment.
LGTM! @bazile-clyde thank you for clarifying the history of this ticket, sounds like people will use it!
kevinAlbs
left a comment
kevinAlbs
left a comment
There was a problem hiding this comment.
Looks good! Just a request for another test case and removal of extra string length calculation.
kevinAlbs
left a comment
kevinAlbs
left a comment
There was a problem hiding this comment.
LGTM! The failures on travis are unrelated.
5 participants
No description provided.