Open vinipsmaker opened 4 years ago
There is a lot to comprehend here, so I will just make some random comments.
Rather than having direct support for optional
and variant
, consider using only supporting dynamic::variable
. dynamic::variable
has converters to different types, including std::vector and boost::optional. Conversion to boost::variant
is probably more tricky, but I think it is doable. Same goes for std::variant
.
Rather than having direct support for
optional
andvariant
, consider using only supportingdynamic::variable
.dynamic::variable
has converters to different types, including std::vector and boost::optional. Conversion toboost::variant
is probably more tricky, but I think it is doable. Same goes forstd::variant
.
There are two sides of this comment. One is interface. Should we keep support for these types at all? Second is implementation. Should we use dynamic::variable
for the heavy work?
For the implementation case, going through dynamic::variable
means an extra indirection. The conversion facilities are cool, but I don't think this is a good use case. The plain pull parser API is actually very easy to use and I don't believe the json::scan()
implementation is substantially more complex because that.
For the interface case, I believe basic type validation is a strong reason behind a scanf()
-like API. If not done there, user will just have to type more right after anyway to check if values match the correct type. Plus types are already there, so why not use them?
Is the current level of “formatted scan” (i.e. embedded type checking) desired? Should we decrease the performed level of type checking and throw some of this work on user's hand? Should we go the other way and increase type checking?
Increasing embedded type checking could mean to add an ADL-like interface in the likes of operator<<(std::ostream&)
where user can teach scan()
how to read user types. After giving thought for this week, I don't think we should go in this direction as it'd mean competing with serialization/reflection libraries and I think that's outside of Trial.Protocol's scope. The ADL interface could be used to adapt to vocabulary types that only exist in the user application (e.g. alternative implementations of std::optional<T>
and std::variant<Ts...>
), but there's always the risk of misuse.
Another step in this direction would be to add support for iarchive
and Boost.Hana's Struct
s (its CT reflection facility). This approach doesn't compete with current serialization/reflection libraries. Rather, it integrates them in. I think that's doable and harm-free, but I'll wait for discussion before implementing anything.
Rather than having direct support for
optional
andvariant
, consider using only supportingdynamic::variable
.dynamic::variable
has converters to different types, including std::vector and boost::optional. Conversion toboost::variant
is probably more tricky, but I think it is doable. Same goes forstd::variant
.For the interface case, I believe basic type validation is a strong reason behind a scanf()-like API. If not done there, user will just have to type more right after anyway to check if values match the correct type.
So, as an example of what I meant by this... the other day I had to implement code to consume permissions and I used dynamic::variable
. I could receive an object like this:
{
"group": {
// ...
}
"action1-perm": "rx",
// ...
}
or an object like this:
{
"group": {
"action1-perm": "rx",
// ...
}
// ...
}
User-level permissions take precedence over group permissions. Therefore, the code would be like:
if (trial::dynamic::key::find(result, "action1-perm") != result.key_end()) {
// ...
} else {
auto group_it = trial::dynamic::key::find(result, "group");
if (group_it == result.key_end()) {
// assign default permission
} else {
// ...
}
}
Rather convoluted code. I'd rather write something like:
std::optional<std::string> user;
std::optional<std::string> group;
json::scan(input, "{ group: { action1-perm: ? }, action1-perm: ? }"_s, group, user);
if (user) {
// ...
} else if (group) {
// ...
} else {
// assign default permission
}
Way cleaner code.
I do not question the utility of json::scan
.
My main concerns are
json::scan
?json::scan
be extended for user-defined types?As an example, serialization addresses both these concerns.
As an example, serialization addresses both these concerns.
I'll open a smaller PR that integrates serialization support and Boost.Hana reflection. When we sort this new PR out, I'll be back to refactor this branch.
As before, I no longer see a need for #26 as I initially proposed. Fields can come in any order and consuming one field means the next desired field might already have been consumed. OTOH, that's not a problem for the
scan()
API as it can freely combine multiple search requests (and there's even wildcard match for other values).
I changed my mind again (partially). I'll drop my new thoughts on #26.
Implementation-wise, everything is done. I do intend to extend the interface, but doing so is an addition (one of the additions is related to this Boost.Hana's issue), not a change. Therefore I consider the PR done as is.
However I'd like to leave it open for a few months while we discuss it.
@xvjau did suggest me to add a
scanf()
like the following:That's one of the intended additions. Before I spend more time extending the API, I'd like to have the basic interface settled as it'll mean less work for me.
What already works (check the test suite for examples):
reader.value<T>()
).boost::optional<T>
.string_view
(the literal is feeded).vector<T>
..std::vector<bool>
doesn't work because it needs special handling (I think I can add it thou if desired)dynamic::variable
.boost::variant<Ts...>
(ifboost::none_t
is inTs...
, then the argument is treated as optional).dynamic::variable
,boost::optional<T>
, orboost::variant<..., boost::none_t, ...>
. I considered making a syntax for optional callbacks, but it added confusion to the interface and the user can just do this with wildcard match on the desired level.partial::scan()
function to parse subtrees).The interface might look a little verbose with all that
boost::hana::map
creation, but this interface actually works great to be further used in another metaprogram. Therefore I consider it desirable and part of the basic API. User might just want to usejson::scan()
implementation while it exposes a completely different interface. In fact, I intend to implement @xvjau's suggestion mentioned above in terms of this very API.As before, I no longer see a need for #26 as I initially proposed. Fields can come in any order and consuming one field means the next desired field might already have been consumed. OTOH, that's not a problem for the
scan()
API as it can freely combine multiple search requests (and there's even wildcard match for other values).So... thoughts?