Moving forward to version 3.0.0

nlohmann commented 7 years ago

I kept postponing the step toward version 3.0.0 for quite some time and quite some issues have been piled up. These issues have in common that fixing them will break the public API - be it very obvious such as removing deprecated functions or changing the type of exceptions or rather subtle such as begin able to store a NaN value.

Question is: How to proceed? So far, I worked in the develop branch. As soon as I, for instance, remove deprecated functions and push to develop, this code may break existing applications. I can live with that - we still have master as a branch containing the code of the last release, yesterday's 2.1.1. Furthermore, OSS-Fuzz for instance, checks the develop branch, so it would be a bit dangerous to only work in a feature branch.

I wonder whether it would be bad style to continue working in develop. What do you think?

ibroheem commented 7 years ago

"I wonder whether it would be bad style to continue working in develop" Thats why its called develop/experimental etc...

How about doing it the GCC way:

4.9.x 5.x.x 6.x.x

Everything in parallel: 2.x.x 3.x.x

Just my own opinion.

theodelrieu commented 7 years ago

That's a good idea, we should separate branches, since we will surely backports bug fixes from 3.0.0 to previous versions.

GCC still releases 4.9.x

nlohmann commented 7 years ago

(We have to keep in mind that the GCC project has far more people, and I could live without 2.x.x releases while preparing for 3.0.0.)

ibroheem commented 7 years ago

2.x.x: No new feature additions, Only fixes. Define an LTS, few months or years. 3.x.x: New features, break the breakable if possible.

gregmarr commented 7 years ago

If you need a definition for what it means to have 3.0.0 vs 2.x.x, I suggest you go with semver.org's definition:

Summary

Given a version number MAJOR.MINOR.PATCH, increment the:

MAJOR version when you make incompatible API changes, MINOR version when you add functionality in a backwards-compatible manner, and PATCH version when you make backwards-compatible bug fixes. Additional labels for pre-release and build metadata are available as extensions to the MAJOR.MINOR.PATCH format.

As for where you do it, I wouldn't expect anyone to be basing production code on a develop branch, so changing that to 3.0.0-alpha and keeping master as 2.x.x until 3.0.0 is ready for master should be fine. If any additional fixes are needed in 2.x.x until that time, a temporary branch could be created for that.

nlohmann commented 7 years ago

@ibroheem I don't think I have the manpower to support 2.x.x for too long...

@gregmarr I like semver - I was just unsure about the actual process and how to implement it with Github. I totally agree that no-one should work trust develop blindly. I just wanted to make sure I'm not ignoring best practices.

TurpentineDistillery commented 7 years ago

I don't think I have the manpower to support 2.x.x for too long...

Even if you expect minimal legacy support for 2.x.x., the branching model should still allow for it, in case you need to make a trivial but important fix now and then.

nlohmann commented 7 years ago

I added a wiki page with notes on the road toward version 3.0.0. I also linked this page in the README file. You all have been warned ;-)

theodelrieu commented 7 years ago

Hi, here are some things I had in mind for 3.0.0:

Reduce the number of `basic_json` template arguments

There is just way too many, the symbols are hard to read, and can crash compilers (especially MSVC) if they become way too complex, (e.g. I had to refrain using detail::disjunction and detail::conjunction).

An easy solution is a single template argument JsonTraits, with all the existing arguments defined inside. (Just like std::iterator_traits)

Right now, it's (almost) impossible to template on basic_json<..., ...>, you need at least 15 typename before writing down the return type, whereas it could simply be:

template <typename Traits, typename T>
void to_json(basic_json<Traits>&, const T&);

Bonus: Forward-declaration becomes really easy, AND it doesn't include map, vector, etc:

namespace nlohmann {
struct json_traits;

template <typename Traits> class basic_json;
using json = basic_json<json_traits>;
}

I could be mistaken, not being an expert with this, but I think updating json_traits won't break anything (from an ABI point-of-view, please correct me if I'm wrong).

We should look at taocpp, I remember one of the maintainers mentioning they were using this technique, during the premises of to/from_json

Split the library into multiple headers

This is mainly a matter of clarity, it's really hard to read through the class (which is at ~12K lines right now). It could also stop possible contributors from jumping in the project.

However I know we want to keep releasing the single json.hpp file for people who wants it. Looking at what Catch does on that matter would be great.

Each basic_json nested struct/class could go into a separate header, and most private methods could land into the detail namespace.

It could look like this (I didn't think about the details like friendship etc):

namespace nlohmann {
namespace detail {
 struct json_pointer {};
}
class basic_json {
public:
using json_pointer = detail::json_pointer;
};
}

About exceptions

I haven't put a lot of thinking into exceptions, but wouldn't it make sense if the only exceptions thrown by the library were derived from json::exception? We could also encapsulate user-defined exceptions with nested exceptions? (I've never tried to use those, but they sound appropriate for that matter)

I'd like to have access to Boost.Outcome or std::expected to satisfy users who disable exceptions, unfortunately that's not possible!

(Minor thing): clang-format

@nlohmann I know you like astyle, but I forget about it everytime! Could you find the time to configure a small clang-format, that approaches your code-beauty standards? If not, that's not a major problem ;)

Well, this post was longer than expected.

jaredgrubb commented 7 years ago

+1 on changing the template to a traits/policy class. I did file an issue on this because I think there's some really cool things you could do with that (#456).

nlohmann commented 7 years ago

@theodelrieu I am currently working at the exceptions. I plan to have a class json::exception with several derived classes like json::parse_error.

nlohmann commented 7 years ago

I shall have a look at clang-format, but it seems to be a lot of work to make it work like astyle.

theodelrieu commented 7 years ago

Hi, I've been thinking about something a bit crazy the past few days!

One of the problems I also have with basic_json is that most of its public static functions could be in a namespace (i.e. most of the code doesn't belong here).

I'd like to keep only constructors, query methods, operator[] and such (i.e relevant parts of a JSON value).

DISCLAIMER This might imply a lot of refactoring, yet I believe this is really feasible, and should be done in a major release.

What I have in mind:

namespace njson {
// this struct exposes each template arg (just like basic_json does: object_t, etc...)
template <...> struct basic_json_traits{/*...*/};
using default_traits = basic_json_traits<>;

template <typename JSONTraits = default_traits>
class basic_value {
public:
  // for 2.x.x compat
  static basic_value parse(...) [[deprecated]] {
    return ::njson::parse<basic_value>(...);
  }

  // and so on for the other methods
};
using value = basic_value<>;

// default traits: auto val = njson::parse(...);
// custom traits: auto val = njson::parse<my_custom_json_value>(...);
template <typename BasicJsonValue = value>
BasicJsonValue parse(...) {/*...*/}
} // namespace njson

// old code will still compile with this
namespace nlohmann [[deprecated]] {
  template <typename T, typename SFINAE>
  using adl_serializer = ::njson::adl_serializer<T, SFINAE>;

  template <...>
  using basic_json = ::njson::basic_value<::njson::basic_json_traits<...>>;

  using json = ::njson::value;
}

There is a slight change on each method call for custom json values, (e.g. parse in this example). I did not rename the namespace to json, it would break code where using nlohmann::json; is used.

I can start a POC on my fork, I'm quite confident that everything should work as expected.

What do you all think about this? I'm waiting for your inputs :)

PS: Now that I think of it, with this change it would be possible to make a static/shared library in the future, with all the internals, and non-template methods we use. I remember some users signaling they wanted this to be possible. But that's really looking far ahead...

nlohmann commented 7 years ago

@theodelrieu We should discuss this in Slack some time.

theodelrieu commented 7 years ago

Sure, I will post on slack at what time I'm available this weekend

gregmarr commented 7 years ago

Is there a slack team/channel for this project?

theodelrieu commented 7 years ago

Here it is, however I think I need to invite your email address. You can send me a mail and I'll do that quickly!

We might set up an IRC channel at freenode, it would be easier for everybody to join, ask questions, and participate to discussions.

nlohmann commented 7 years ago

Let's please use Slack - opening a second channel would just mean more work...

theodelrieu commented 7 years ago

That's right, my bad

nlohmann commented 7 years ago

This may be on short notice, but I am in Slack (https://nlohmannjson.slack.com/messages/general) now and most likely for the next 3 hours. @theodelrieu @gregmarr and everybody, feel free to join!

nlohmann commented 7 years ago

@theodelrieu I thought about your proposal to reorganize the classes; that is, move most of them out of basic_json into the detail namespace and to re-introduce them to basic_json with using.

Here is the current structure:

namespace nlohmann
- namespace detail
  - class exception
  - class parse_error
  - class invalid_iterator
  - class type_error
  - class out_of_range
  - class other_error
- class basic_json
  - class serializer (private, 585 LOC)
  - class primitive_iterator_t (private, 134 LOC)
  - struct internal_iterator (private, 22 LOC)
  - class iteration_proxy (private, 96 LOC)
  - class iter_impl (616 LOC)
  - class json_reverse_iterator (112 LOC)
  - class lexer (private, 848 LOC)
    - struct strtonum
  - class parser (private, 305 LOC)
  - class json_pointer (679 LOC)

I added the lines of code to support your observation that there is a lot of code that could be moved out of the way. However, I also found some ways to group things along the aspects they address. Furthermore, it would be helpful to also move everything related to CBOR/MessagePack as well as JSON Pointer/Patch out of the way:

exceptions:
- class exception
- class parse_error
- class invalid_iterator
- class type_error
- class out_of_range
- class other_error
iterators:
- class iteration_proxy (private)
- class primitive_iterator_t (private)
- struct internal_iterator (private)
- class iteration_proxy (private)
- class iter_impl
deserialization:
- class lexer (private)
  - struct strtonum
- class parser (private)
serialization:
- class serializer (private)
binary formats (1789 LOC):
- cbor (should be moved in class)
- msgpack (should be moved in class)
JSON pointer / patch
- json_pointer
- json_patch (315 LOC, should be moved in class)

I would like to discuss how to restructure this logically. Moving classes into separate files would then be the next step.

theodelrieu commented 7 years ago

This seems good to me!

I will have more time to discuss next week, meanwhile I'll try to work a bit on my fork.

JoveToo commented 7 years ago

Perhaps some of the deprecated code could also be moved do a "deprecated" namespace and require an explicit "using" to bring it back. Document it well and it will break peoples builds only once after upgrading to a new version (which is when they should expect issues like this).

nlohmann commented 7 years ago

FYI: With the latest commit, I merged a feature branch with a manual lexer. That is, the code does not use a state machine generated by re2c, but a hand-written state machine. In addition, I implemented "input adapters" separate the actual input (e.g., stream, string, byte vector) from the lexer. This simplified a the code in a lot of places and avoids error-prone refilling of an intermediate buffer that was required by re2c.

theodelrieu commented 7 years ago

Nice to hear! Is it a good time to start splitting the basic_json class? I'm not talking about splitting the code into multiple files.

nlohmann commented 7 years ago

Opened #623 as place to discuss the structure of the parsing functions.

theodelrieu commented 7 years ago

Hi, I've been thinking a bit about all the template arguments that basic_json takes.

The intent was to be "generic", and let users decide which string, object type (and others) they wanted to use.

One of the issue is that in the implementation, there's often lots of constraints about the API of those types.

Right now, you cannot use something else than std::string (I've even seen a function that returns std::to_string(...)). ObjectType must have a key_type and a mapped_type, etc...

Although there are some people that may want to handle floats themselves (e.g. #596) I doubt someone ever specialized NumberBooleanType.

Furthermore, IIRC the AllocatorType just does not work.

So, my question is: Which template arguments are really relevant/useful?

stale[bot] commented 6 years ago

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.

meox commented 6 years ago

any news on this release?

nlohmann commented 6 years ago

I wish... I would like to get this release out this year, but I don't have much time for big changes at the moment. So far, I would rather have something released soon just to have develop and master be in sync again.

nlohmann commented 6 years ago

Todo:

[x] better document iteration_wrapper
[x] add all recent contributors to README
[x] add exception for #838
[x] example for throwing dump
~~[ ] documentation of semver/deprecation~~

nlohmann commented 6 years ago

I shall release 3.0.0 tomorrow.

nlohmann / json