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

Refactor/split it #700

Merged
merged 23 commits into from
Jan 9, 2018
Merged

Refactor/split it #700

merged 23 commits into from
Jan 9, 2018

Conversation

theodelrieu
Copy link
Contributor

Here it is.

refactortime

This PR is the follow-up of #643, and the second phase of my planned refactoring (there will likely be one more, but we shall discuss it in a separate thread, since it requires naming/structural changes).

I've added a tool to amalgamate the library into a single header (like Catch does), @nlohmann pointed out this was necessary.

There is a new rule in the Makefile: single_include, that builds, and executes the tool.

The output is located in the single_include folder, at the root of the repo.

The only requirement is that every included file that is part of the library must be with the following style:

#include <will_not_be_inlined.hpp>
#include "will_be_inlined.hpp"

Each include must be done with a path relative to the source folder:

// detail/conversions/from_json.hpp

#include "detail/value_t.hpp" // good
#include "../value_t.hpp" // NOPE, not refactor-proof

I've also added one test for each source file (except json.hpp), to check that we didn't miss any include.
This will be helpful for future refactoring.

@theodelrieu
Copy link
Contributor Author

Well, looks like codacy wakes up a bit late!

@theodelrieu theodelrieu force-pushed the refactor/split_it branch 2 times, most recently from 444b7b7 to 97ee19a Compare August 14, 2017 19:49
@nlohmann
Copy link
Owner

Wow, this is a big PR... I am not sure yet whether I will be able to find my way around in the code. Let's see.

@theodelrieu
Copy link
Contributor Author

theodelrieu commented Aug 14, 2017

Don't worry, this is just code moving from json.hpp to different files ;)

Edit: The +50 000 lines are from the tools actually ... that might be a problem, we could move it from the repo (a submodule would do the job, even if I'm not very fond of them)

@nlohmann
Copy link
Owner

I know - but I got used to that one large file!

@theodelrieu
Copy link
Contributor Author

Ahah time to let it go!

@nlohmann
Copy link
Owner

Let's see. Maybe I see the benefits soon.

@theodelrieu
Copy link
Contributor Author

Well one very big advantage is that people can work on different areas of the code much more easily, before, rebasing was just impossible (I tried, and it hurt).

Also, after phase 3, it will be possible for users to only include what they want (e.g. the file where json::parse resides)

It's not possible yet, I didn't touch at basic_json, but once (almost) all its methods will be moved in a namespace, it will be.

@theodelrieu
Copy link
Contributor Author

Do you have any idea on travis failure? I don't get why the clang_sanitize fails.

By the way, do you know if I can cancel Travis build? They take soooo long, and if I repush twice in a row, I have to wait 2 full hours to know if my last commit passed...

@theodelrieu
Copy link
Contributor Author

Apparently, -fsanitize=address is not recognized by clang...

@nlohmann
Copy link
Owner

I overworked the Travis configuration. With 21726d8, the sanitizier should work again.

@theodelrieu
Copy link
Contributor Author

Great! I will trigger Travis once I arrive to work. As for Codacy, I'd rather fix its warnings after this PR, as they were introduced before.

I'll also rename commits to reflect the actual paths (omitted detail).

@theodelrieu theodelrieu force-pushed the refactor/split_it branch 2 times, most recently from 5410be8 to c9f38ad Compare August 16, 2017 09:41
@theodelrieu
Copy link
Contributor Author

Hmm, coveralls seems to be hanging, is there some configuration files to modify too?

@theodelrieu
Copy link
Contributor Author

I have no idea why VS2017 fails, I ran the same commands than AppVeyor on my laptop, and all tests pass...

@nlohmann
Copy link
Owner

I have the same problems with AppVeyor in the develop branch. :-S

@nlohmann
Copy link
Owner

@theodelrieu
Copy link
Contributor Author

Looks promising, thanks!

@nlohmann
Copy link
Owner

AppVeyor works now.

@theodelrieu
Copy link
Contributor Author

Thanks, I don't know about Coveralls though, but I just moved code around, so I don't believe it would be relevant if it was up.

As I mentioned earlier about Codacy, I'd like to fix its warnings after the PR, since they are related to previous commits.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 7e4ee23 on theodelrieu:refactor/split_it into 15b6421 on nlohmann:develop.

@nlohmann nlohmann self-assigned this Jan 9, 2018
@nlohmann nlohmann added this to the Release 3.1.0 milestone Jan 9, 2018
@nlohmann nlohmann merged commit b67e00b into nlohmann:develop Jan 9, 2018
@nlohmann
Copy link
Owner

nlohmann commented Jan 9, 2018

Thanks a lot!!!

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.

6 participants