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

Change header include directory to nlohmann/json #942

Closed
patrikhuber opened this issue Jan 29, 2018 · 4 comments
Closed

Change header include directory to nlohmann/json #942

patrikhuber opened this issue Jan 29, 2018 · 4 comments
Labels
solution: proposed fix a fix for the issue has been proposed and waits for confirmation
Milestone

Comments

@patrikhuber
Copy link
Contributor

Hi!

The readme advises to include the header with #include "json.hpp". If we add this repo as a git submodule, we would add the src/ directory of this repo to the include path, and also do #include "json.hpp". However, if one runs the CMake INSTALL target of this repo, or uses it via e.g. vcpkg, the header gets copied to the location nlohmann/json.hpp and should now be included as #include "nlohmann/json.hpp".
I think it would be better (and best practice) to put the header in the repo into a directory include/nlohmann/. Most header-only libraries that I know of do this. This would unify the #include: We could just use #include "nlohmann/json.hpp, no matter whether we're including via git submodule, CMake INSTALL target, vcpkg, etc. Another plus is that it sort-of "namespaces" the header, i.e. a clash with a potential other json.hpp file in any of the compiler's include directories is avoided (not unlikely, as json.hpp is quite a generic name).

@nlohmann
Copy link
Owner

I am afraid that changing the path of the single header from src/json.hpp to anything else would also break existing systems.

@nlohmann nlohmann added the state: please discuss please discuss the issue or vote for your favorite option label Jan 29, 2018
@nlohmann
Copy link
Owner

Ok. This will be fixed with #944.

@nlohmann nlohmann removed the state: please discuss please discuss the issue or vote for your favorite option label Jan 31, 2018
@nlohmann nlohmann added this to the Release 3.1.0 milestone Jan 31, 2018
@patrikhuber
Copy link
Contributor Author

Great to see this awesome discussion on that matter! The PR will make me very happy as well ;-) 👍

@nlohmann
Copy link
Owner

nlohmann commented Feb 1, 2018

Fixed with #944.

@nlohmann nlohmann closed this as completed Feb 1, 2018
@nlohmann nlohmann added the solution: proposed fix a fix for the issue has been proposed and waits for confirmation label Feb 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
solution: proposed fix a fix for the issue has been proposed and waits for confirmation
Projects
None yet
Development

No branches or pull requests

2 participants