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

Make integration section concise #981

Merged
merged 1 commit into from
Feb 26, 2018
Merged

Conversation

wla80
Copy link
Contributor

@wla80 wla80 commented Feb 23, 2018

Rewrite in concise sentences to improve the readability of the section

@coveralls
Copy link

coveralls commented Feb 24, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling 05d3bf1 on wla80:develop into 8968adc on nlohmann:develop.

README.md Outdated
@@ -64,9 +64,9 @@ The single required source, file `json.hpp` is in the `single_include/nlohmann`
using json = nlohmann::json;
```

to the files you want to use JSON objects. That's it. Do not forget to set the necessary switches to enable C++11 (e.g., `-std=c++11` for GCC and Clang).
to the files you want to use JSON objects and set the necessary switches to enable C++11 (e.g., `-std=c++11` for GCC and Clang).
Copy link
Owner

Choose a reason for hiding this comment

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

"JSON objects" sounds odd here (I know it was in my original sentence...). "Object" already has a meaning in JSON - maybe "to the files you want to use JSON in"? Any ideas?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the feedback. This sounds OK but how about "to the files you want to process JSON". Also, you had used "JSON object" multiple times in this doc.

Copy link
Owner

Choose a reason for hiding this comment

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

"process JSON" is even better. I have to go through the README again to check where I used "JSON object" in a different sense than "a JSON value of type object"...

README.md Outdated

You can further use file [`include/nlohmann/json_fwd.hpp`](https://github.com/nlohmann/json/blob/develop/include/nlohmann/json_fwd.hpp) for forward-declarations. The installation of json_fwd.hpp (as part of cmake's install step), can be achieved by setting `-DJSON_MultipleHeaders=ON`:
You can install [`include/nlohmann/json_fwd.hpp`](https://github.com/nlohmann/json/blob/develop/include/nlohmann/json_fwd.hpp) by setting `-DJSON_MultipleHeaders=ON` as a part of cmake's install step for forward-declarations.
Copy link
Owner

Choose a reason for hiding this comment

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

-DJSON_MultipleHeaders=ON is not for installing forward-declarations - this is just a side effect. I wanted to note there exists a forward-declaration header. And that it is already part of the CMake project with those flags.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry about this. If this is the case, I will keep the original version.

Copy link
Owner

Choose a reason for hiding this comment

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

OK.

@nlohmann
Copy link
Owner

@wla80 Could you comment on the remarks above?

@wla80
Copy link
Contributor Author

wla80 commented Feb 26, 2018

Hi @nlohmann , I just uploaded the changes based on our earlier discussion. Please check and thank you

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 self-assigned this Feb 26, 2018
@nlohmann nlohmann added this to the Release 3.1.2 milestone Feb 26, 2018
@nlohmann nlohmann merged commit 13ca723 into nlohmann:develop Feb 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants