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

Consequences of from_json / to_json being in namespace of data struct. #1042

Closed
Anketell opened this issue Apr 8, 2018 · 3 comments
Closed
Labels
solution: wontfix the issue will not be fixed (either it is impossible or deemed out of scope)

Comments

@Anketell
Copy link

Anketell commented Apr 8, 2018

Hi,

I was recently shown nlohmann::json and have found it to be very nice.

I have a query surrounding the necessity of from_json / to_json converters to be in a data structure's namespace. Primarily I was wondering what the benefit was of this decision? Below I've illustrated the consequences in an example that cannot easily be addressed with this constraint.

Example

Pulling share trading data from two web portals that publish in JSON. There is a common struct used in more agnostic code:

namespace shares
{
   struct price_t
   {
      uint64_t time;
      double   open;
      double   high;
      double   low;
      double   close;
      uint64_t volume;
   };
}

One site published this data in JSON as an object:

{
   "time": xxxx,
   "open": xxxx,
   "high": xxxx,
   "low": xxxx,
   "close": xxxx,
   "volume": xxxx
}

The second as an array:

[ xxxx, xxxx, xxxx, xxxx, xxxx, xxxx ]

Given that I must define from_json in namespace shares, I cannot conveniently define multiple from_json converters, one for each schema.

Ideally I'd like to define the following:

namespace schema_1
{
   void from_json( const nlohmann::json & value, shares::price_t & price )
   {
      price.time = value[ "time" ];
      price.open = value[ "open" ];
      price.high = value[ "high" ];
      price.low = value[ "low" ];
      price.close = value[ "close" ];
      price.volume = value[ "volume" ];
   }
}

And:

namespace schema_2
{
   void from_json( const nlohmann::json & value, shares::price_t & price )
   {
      price.time = value[ 0 ];
      price.open = value[ 1 ];
      price.high = value[ 2 ];
      price.low = value[ 3 ];
      price.close = value[ 4 ];
      price.volume = value[ 5 ];
   }
}

Then use them like this:

shares::price_t read_schema_1( const std::string & url )
{
   using schema_1::from_json;

   return curl.read_json( url );
}

And:

shares::price_t read_schema_2( const std::string & url )
{
   using schema_2::from_json;

   return curl.read_json( url );
}

In general the necessity to place from_json and to_json in the the namespace of the data structure couples that data structure too tightly to an externally defined JSON schema. This results in negatively limiting its flexibility.

Unless there is some compelling reason for binding these conversion routines in this way, I'd suggest that this limitation be removed.

@Anketell Anketell changed the title Consequences of from_json / to_json being members of namespace of data structure. Consequences of from_json / to_json being in namespace of data struct. Apr 8, 2018
@jaredgrubb
Copy link
Contributor

That "limitation" is not created by this project. That's just the way that "ADL" (Argument Dependent Lookup) works in C++ and is the standard way to implement customization points like this. I don't believe the syntax you're suggesting is actually possible in C++, but I'm happy to be proved wrong if you have a patch :)

@theodelrieu
Copy link
Contributor

Yeah, I don't think there's a way for the library to do anything.

My guess would be to perform the is_array check inside the from_json function.

@nlohmann nlohmann added the solution: wontfix the issue will not be fixed (either it is impossible or deemed out of scope) label Apr 10, 2018
@Anketell
Copy link
Author

Anketell commented Apr 15, 2018

@jaredgrubb "ADL" is a scope relaxation mechanism, not a scope constraining mechanism. However, I do see your point that "ADL" is what the library relies upon to resolve from_json / to_json from within an otherwise more constraining scope.

@theodelrieu thanks for you suggestion, it's a possibility and for some circumstances it might be the only practical solution. If for instance when the schema cannot be determined except by examination. However, it suffers from loss of encapsulation of a schema unless each has a conformance test. JSON schema might be an extension that would help here.

I have a couple of approaches that work when the schema is previously known. Firstly:

namespace schema_1
{
   class from
   {
      const json & m_json;

   public:

      from( const json & j ) : m_j( j ) {}

      operator shares::price_t ( void )
      {
         shares::price_t price;

         price.time = m_json[ "time" ];
         price.open = m_json[ "open" ];
         price.high = m_json[ "high" ];
         price.low = m_json[ "low" ];
         price.close = m_json[ "close" ];
         price.volume = m_json[ "volume" ];

         return price;
      }
   };
}

Then:

shares::price_t get_schema_1( const json & j )
{
   return schema_1::from( j );
}

Obviously a schema_1::to class can also be defined overloading operator=.

And, secondly:

namespace schema_2
{
   const json & operator >> ( const json & j, shares::price_t & price )
   {
      price.time = j[ 0 ];
      price.open = j[ 1 ];
      price.high = j[ 2 ];
      price.low = j[ 3 ];
      price.close = j[ 4 ];
      price.volume = j[ 5 ];

      return j;
   }
}

Then:

shares::price_t get_schema_2( const json & j )
{
   using namespace schema_2;

   shares::price_t price;

   j >> price;

   return price;
}

And obviously operator<< can also be defined.

Neither provide the syntactic sweetness of an "ideal" but they both offer the advantage that a namespace can contain all the conversions to and from a schema - encapsulating it.

For my money I favour the former over the later as its usage is more concise but would be interested in other suggestions / improvements.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
solution: wontfix the issue will not be fixed (either it is impossible or deemed out of scope)
Projects
None yet
Development

No branches or pull requests

4 participants