Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add json{::partial,}::scan() in the likes of the scanf() API #43

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

vinipsmaker
Copy link
Contributor

@vinipsmaker vinipsmaker commented Jun 14, 2020

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:

using namespace boost::hana::literals;
int foo_bar, bar;
json::scan(input, "{ foo: { bar: ? }, bar: ? }"_s, foo_bar, bar);

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):

  • Reading to a simple variable (i.e. the usual stuff from reader.value<T>()).
  • Reading to a boost::optional<T>.
  • Reading to a string_view (the literal is feeded).
  • Reading to a vector<T>. std::vector<bool> doesn't work because it needs special handling (I think I can add it thou if desired).
  • Recursive combinations of the above.
  • Reading to dynamic::variable.
  • Reading to a boost::variant<Ts...> (if boost::none_t is in Ts..., then the argument is treated as optional).
  • Calling a callback.
  • Any argument is treated as required unless it is dynamic::variable, boost::optional<T>, or boost::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.
  • Arbitrary nesting (and a partial::scan() function to parse subtrees).
  • Wildcard matching on any desired level. Action can only be a callback and it is treated as non-required.
  • Error reporting is done through exceptions. If desired, I can add information on missing fields to the exception (the only trick I'll need to pay attention to is reports from subtrees), but this requires me to design the exception class to hold such fields and I'd like to discuss that.

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 use json::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?

@breese
Copy link
Owner

breese commented Jun 14, 2020

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.

@vinipsmaker
Copy link
Contributor Author

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.

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 Structs (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.

@vinipsmaker
Copy link
Contributor Author

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.

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.

@breese
Copy link
Owner

breese commented Aug 9, 2020

I do not question the utility of json::scan.

My main concerns are

  1. Dependencies: Do we need to include a universe of header files to use json::scan?
  2. Customization: Can json::scan be extended for user-defined types?

As an example, serialization addresses both these concerns.

@vinipsmaker
Copy link
Contributor Author

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.

@vinipsmaker
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants