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

basic_json templated on a "policy" class #456

Closed
jaredgrubb opened this issue Feb 15, 2017 · 7 comments
Closed

basic_json templated on a "policy" class #456

jaredgrubb opened this issue Feb 15, 2017 · 7 comments
Labels
kind: enhancement/improvement state: please discuss please discuss the issue or vote for your favorite option state: stale the issue has not been updated in a while and will be closed automatically soon unless it is updated

Comments

@jaredgrubb
Copy link
Contributor

jaredgrubb commented Feb 15, 2017

I'm enjoying the library, however I have a couple comments:

  • the mangled name is really awful:
    nlohmann::basic_json<std::__1::map, std::__1::vector, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, bool, long long, unsigned long long, double, std::__1::allocator, nlohmann::adl_serializer>
  • it's not possible to extend basic_json beyond the types

The mangled name is cosmetic, but it is annoying to have to try to read stackshots or prints in debuggers.

One idea I had was to refactor basic_json to be based on a policy type, aka nlohmann::basic_json<nlohmann::default_policy>, where default_policy would be defined something like:

template <
    template<typename U, typename V, typename... Args> class ObjectType = std::map,
    template<typename U, typename... Args> class ArrayType = std::vector,
    class StringType = std::string,
    class BooleanType = bool,
    class NumberIntegerType = std::int64_t,
    class NumberUnsignedType = std::uint64_t,
    class NumberFloatType = double,
    template<typename U> class AllocatorType = std::allocator,
    template<typename T, typename SFINAE = void> class JSONSerializer = adl_serializer
    >
struct policy_base {
    using object_t = ...;
    using array_t = ...;
    // ...

    using json_value = ... /* union definition */;
    using parser = ...;

    // ... whatever accessors are needed to complete the abstraction
};

struct default_policy : policy_base<> {};

It would take a bit of work to get basic_json refactored in terms of the policy, and to figure out exactly what should go in each part. But I think this could make the design a bit cleaner in that it turns basic_json into the interface-glue that makes the type easy to use, and the policy object becomes the low-level details that provide a nice customization point.

The kinds of customizations that this could enable:

  • allow the union to be extended to support user types (for example, std::shared_ptr<json>, or std::unique_ptr<LazyJsonLoader> that only deserializes a sub-object if it gets used, or to allow EmployeeRecord to be placed directly in the union and serialized inline, but also provide conversion-operators like other types)
  • maybe the parser could be extended to support non-conforming extensions (like trailing comma); I support that this library doesnt support this out of the box, but it would be nice if it was doable without too much work. (I have not looked at the parser, so maybe this is not even remotely possible right now).

Anyway, I just wanted to throw the idea out there to get feedback.

@gregmarr
Copy link
Contributor

gregmarr commented Feb 15, 2017

You can get around the debugging issue with something like what's mentioned in this thread:
#314

namespace marton78
{
    struct json : nlohmann::basic_json<>
    {
        using basic_json<>::basic_json;
        json(basic_json<> const& j) : basic_json<>(j) {}
    };
}

@nlohmann
Copy link
Owner

I understand your points, but I currently have no capacity for such a refactoring right now. I think this would be a change in the public API, so this cannot go into a 3.x.y release. So maybe this is something for the 4.0.0.

@jaredgrubb
Copy link
Contributor Author

Do you find the approach reasonable, if it was done in a way that does not break the public API? I would be willing to try to make a patch. It's not trivial so I didn't really want to do the work unless you think it's something you would want.

@nlohmann
Copy link
Owner

I'm not sure about this yet to be honest.

@nlohmann nlohmann added the state: please discuss please discuss the issue or vote for your favorite option label Feb 15, 2017
@nlohmann
Copy link
Owner

nlohmann commented Jun 5, 2017

Maybe related: #599

@nlohmann
Copy link
Owner

Related: nlohmann/std_json@a054558

@stale
Copy link

stale bot commented Oct 25, 2017

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the state: stale the issue has not been updated in a while and will be closed automatically soon unless it is updated label Oct 25, 2017
@stale stale bot closed this as completed Nov 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: enhancement/improvement state: please discuss please discuss the issue or vote for your favorite option state: stale the issue has not been updated in a while and will be closed automatically soon unless it is updated
Projects
None yet
Development

No branches or pull requests

3 participants