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

Refacto/split basic json #643

Merged
merged 9 commits into from
Jul 9, 2017

Conversation

theodelrieu
Copy link
Contributor

Hi, I managed to move every nested struct/class present in basic_json to the detail namespace.

Excepted json_pointer which is now in nlohmann, I don't know if input/output adapters are exposed, if so, I will move them there too.

This is an intermediate refactoring, needed before splitting the library into multiple files, and before changing classes and namespace names.

I'm afraid the review will not be very interesting... I advise reviewers to look at each commit separately, it is not readable otherwise.

@theodelrieu
Copy link
Contributor Author

It's weird that codacy only complains now, I did not change this code.

@theodelrieu
Copy link
Contributor Author

theodelrieu commented Jul 6, 2017

I forgot to use the public API in every class i've split (except parser that became a friend, since it needs to modify m_type/m_value directly).

The tests passed because of the #define private public line present in almost each test file.

I think we should get rid of this line, it's quite dangerous when refactoring

@nlohmann
Copy link
Owner

nlohmann commented Jul 6, 2017

I added this to allow for 100% coverage without changing the code. I'm not a fan of code refactoring just to simplify testing. I need to have a look at this PR when I'm home.

@theodelrieu
Copy link
Contributor Author

Please note that the purpose of this PR is not to facilitate testing, but to prepare the split of json.hpp.

The main reason why this line exists is that we test private nested classes' public APIs. Once we split the code into multiple files, we'll be able to unit test each component public API directly, without decreasing today's coverage rate.

@nlohmann
Copy link
Owner

nlohmann commented Jul 6, 2017

Sorry, i misunderstood. I shall have a look on Saturday.

@theodelrieu theodelrieu force-pushed the refacto/split_basic_json branch 5 times, most recently from f925060 to 22b4f53 Compare July 7, 2017 11:48
@theodelrieu
Copy link
Contributor Author

It's weird, the unit-cbor.cpp file compiles on my laptop. I had to compile it by hand though, it seems it's not included when running ninja

Those macros are used to reduce template argument boilerplate
@theodelrieu theodelrieu force-pushed the refacto/split_basic_json branch 2 times, most recently from 7e2a199 to c2cef9c Compare July 9, 2017 16:19
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 8585d35 on theodelrieu:refacto/split_basic_json into d349634 on nlohmann:develop.

@theodelrieu
Copy link
Contributor Author

Finally I made that pass \o/

@nlohmann If you could take a look before pushing other commits, it'd be great (rebasing is horrible with all the code moving)

Also I won't be available for the next 2 weeks, so I can only make changes tonight (sorry for the rush :p)

@nlohmann
Copy link
Owner

nlohmann commented Jul 9, 2017

Sorry for changing to much lately. I kind of forgot what it means to pending PRs...

Copy link
Owner

@nlohmann nlohmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@nlohmann nlohmann merged commit 7dee868 into nlohmann:develop Jul 9, 2017
@nlohmann nlohmann self-assigned this Jul 9, 2017
@nlohmann nlohmann added this to the Release 3.0.0 milestone Jul 9, 2017
@theodelrieu
Copy link
Contributor Author

Thanks a lot! We shall talk about the next steps after I come back of vacations (I'll be available on slack during that time anyway)

The first thing should be quite trivial: splitting into multiple files, and add a cmake/makefile rule to amalgamate a single json.hpp file. Looking how Catch does that should be helpful.

The second is more hard, it's about the API breakage (namespace naming, single traits class, etc...) as discussed in #474.

I'll be happy to get on that issue when I come back!

@nlohmann
Copy link
Owner

nlohmann commented Jul 9, 2017

Have a nice vacation!

@theodelrieu theodelrieu mentioned this pull request Aug 14, 2017
@dan-42 dan-42 mentioned this pull request Sep 19, 2017
@theodelrieu theodelrieu deleted the refacto/split_basic_json branch March 13, 2019 08:45
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.

3 participants