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

Allow custom base class as node customization point #3110

Conversation

barcode
Copy link
Contributor

@barcode barcode commented Oct 30, 2021

This PR adds an additional template parameter which allows to set a custom base class for nlohmann::json. This class serves as an extension point and allows to add functionality to json node. Examples for such functionality might be metadata or additional member functions (e.g. visitors) or other application specific code.
By default the parameter is set to void and an empty base class is used. In this case the library behaves as it already did.


Pull request checklist

Read the Contribution Guidelines for detailed information.

  • Changes are described in the pull request, or an existing issue is referenced.
  • The test suite compiles and runs without error.
  • Code coverage is 100%. Test cases can be added by editing the test suite. (should be. will update state aver coverall is done)
  • The source code is amalgamated; that is, after making changes to the sources in the include/nlohmann directory, run make amalgamate to create the single-header file single_include/nlohmann/json.hpp. The whole process is described here.

@barcode barcode requested a review from nlohmann as a code owner October 30, 2021 18:02
@barcode barcode mentioned this pull request Oct 30, 2021
4 tasks
@barcode barcode force-pushed the allow_custom_base_class_as_node_customization_point branch from f3671da to 8ed9dd7 Compare October 30, 2021 18:16
@barcode
Copy link
Contributor Author

barcode commented Oct 30, 2021

Just saw i used the wrong base branch and fixed it.

@coveralls
Copy link

coveralls commented Oct 30, 2021

Coverage Status

Coverage remained the same at 100.0% when pulling 46597f0 on barcode:allow_custom_base_class_as_node_customization_point into 8fcdbf2 on nlohmann:develop.

@barcode barcode force-pushed the allow_custom_base_class_as_node_customization_point branch from 8ed9dd7 to 471b245 Compare October 30, 2021 19:24
@nlohmann
Copy link
Owner

Not sure about the cppcheck error, but there are also issues with MSVC.

@nlohmann
Copy link
Owner

Great! Windows seem to work now. The amalgamation check failed though. Please try again after calling make amalgamate.

@barcode
Copy link
Contributor Author

barcode commented Oct 31, 2021

For some strange reason make amalgamate did not do anything (make: Nothing to be done for 'amalgamate'.). After deleting the single include and running make amalgamate again, it also did some formatting. Not sure if that was supposed to happen.

The cppcheck test was skipped this time, so it did not fail. I guess it will fail again.
Not sure why it is complaining about, but it had the same issue on my old PR

Check code with Cppcheck
Checking /__w/json/json/single_include/nlohmann/json.hpp ...
/__w/json/json/single_include/nlohmann/json.hpp:3448:54: error: syntax error [syntaxError]
         class BinaryType = std::vector<std::uint8_t>,
                                                     ^

The ci uses cppcheck 2.7. This seems to be some version of the current main branch. If i build cppcheck main and use it, i get the same error. It looks like a false positive. I could suppress it using // cppcheck-suppress syntaxError.

@nlohmann
Copy link
Owner

For some strange reason make amalgamate did not do anything (make: Nothing to be done for 'amalgamate'.). After deleting the single include and running make amalgamate again, it also did some formatting. Not sure if that was supposed to happen.

Unfortunately, this can happen. You fixed it the right way. Sorry for the inconvenience.

The ci uses cppcheck 2.7. This seems to be some version of the current main branch. If i build cppcheck main and use it, i get the same error. It looks like a false positive. I could suppress it using // cppcheck-suppress syntaxError.

Yes, the CI uses the bleeding-edge version of cppcheck. If you can suppress the issue, then I guess this is the way.

@barcode
Copy link
Contributor Author

barcode commented Nov 1, 2021

Finally all checks went green. All but the first commit were fixes for the CI and do not carry important information. I think merge+squash would be a good way to keep the commit history in a readable form.

Not sure why it displayed a green tick earlier...

@nlohmann
Copy link
Owner

nlohmann commented Nov 1, 2021

std::make_unique is not available in C++11 - I guess that's why the CI fails.

include/nlohmann/detail/json_custom_base_class.hpp Outdated Show resolved Hide resolved
test/src/unit-custom-base-class.cpp Outdated Show resolved Hide resolved
test/src/unit-custom-base-class.cpp Outdated Show resolved Hide resolved
@barcode barcode requested a review from nlohmann November 1, 2021 17:05
include/nlohmann/detail/json_custom_base_class.hpp Outdated Show resolved Hide resolved
test/src/unit-custom-base-class.cpp Outdated Show resolved Hide resolved
@barcode barcode requested a review from nlohmann November 1, 2021 20:04
@nlohmann nlohmann added the review needed It would be great if someone could review the proposed changes. label Nov 4, 2021
@stale
Copy link

stale bot commented Jan 9, 2022

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.

@stale stale bot added the state: stale the issue has not been updated in a while and will be closed automatically soon unless it is updated label Jan 9, 2022
@gregmarr
Copy link
Contributor

@barcode @nlohmann I think this just needs some documentation.

@stale stale bot removed the state: stale the issue has not been updated in a while and will be closed automatically soon unless it is updated label Jun 18, 2022
@barcode barcode force-pushed the allow_custom_base_class_as_node_customization_point branch from 3311d11 to e0a30ee Compare June 28, 2022 08:33
@barcode
Copy link
Contributor Author

barcode commented Jun 28, 2022

@nlohmann @gregmarr
I have rebased the branch on develop and added some API documentation.
Let me know, if there is anything else stopping this PR from getting merged.

docs/examples/json_base_class_t.cpp Show resolved Hide resolved
include/nlohmann/json.hpp Outdated Show resolved Hide resolved
include/nlohmann/json_fwd.hpp Show resolved Hide resolved
tests/src/unit-custom-base-class.cpp Show resolved Hide resolved
tests/src/unit-custom-base-class.cpp Outdated Show resolved Hide resolved
docs/mkdocs/docs/api/basic_json/json_base_class_t.md Outdated Show resolved Hide resolved
docs/mkdocs/docs/api/basic_json/json_base_class_t.md Outdated Show resolved Hide resolved
docs/mkdocs/docs/api/basic_json/json_base_class_t.md Outdated Show resolved Hide resolved
@falbrechtskirchinger
Copy link
Contributor

@nlohmann Please approve the workflow run.

@barcode
Copy link
Contributor Author

barcode commented Jul 12, 2022

Strange... not sure why the previous run of cppcheck accepted the code without the suppress comment. I guess it is a sporadic error. Hence i added the comment again.

@nlohmann nlohmann added the please rebase Please rebase your branch to origin/develop label Aug 7, 2022
@barcode barcode force-pushed the allow_custom_base_class_as_node_customization_point branch from 5e700cc to 1b983c8 Compare August 8, 2022 21:47
Copy link
Contributor

@falbrechtskirchinger falbrechtskirchinger 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.

@falbrechtskirchinger
Copy link
Contributor

falbrechtskirchinger commented Aug 27, 2022

Some notes for a follow-up PR:

  • Make specifying a base class easier/less verbose.
  • Warn users about potential conflicts with newer versions, i.e., base members should avoid generic names to avoid clashes with future member additions to basic_json.
  • Doxygen comments should conform to the "new" convention.
  • (Run reuse.)

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.

Some minor comments, but otherwise looks good to me.

docs/mkdocs/docs/api/basic_json/json_base_class_t.md Outdated Show resolved Hide resolved
docs/mkdocs/docs/api/basic_json/json_base_class_t.md Outdated Show resolved Hide resolved
docs/mkdocs/docs/api/basic_json/json_base_class_t.md Outdated Show resolved Hide resolved
docs/mkdocs/docs/api/basic_json/json_base_class_t.md Outdated Show resolved Hide resolved
docs/mkdocs/docs/api/basic_json/json_base_class_t.md Outdated Show resolved Hide resolved
include/nlohmann/detail/json_custom_base_class.hpp Outdated Show resolved Hide resolved
barcode and others added 6 commits August 27, 2022 17:11
Co-authored-by: Niels Lohmann <niels.lohmann@gmail.com>
Co-authored-by: Niels Lohmann <niels.lohmann@gmail.com>
Co-authored-by: Niels Lohmann <niels.lohmann@gmail.com>
Co-authored-by: Niels Lohmann <niels.lohmann@gmail.com>
Co-authored-by: Niels Lohmann <niels.lohmann@gmail.com>
Co-authored-by: Niels Lohmann <niels.lohmann@gmail.com>
@github-actions
Copy link

🔴 Amalgamation check failed! 🔴

The source code has not been amalgamated.

Co-authored-by: Niels Lohmann <niels.lohmann@gmail.com>
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
Copy link
Owner

Will merge once the CI is green.

@nlohmann nlohmann added this to the Release 3.11.3 milestone Aug 27, 2022
@github-actions
Copy link

🔴 Amalgamation check failed! 🔴

The source code has not been amalgamated.

@barcode barcode requested a review from nlohmann August 28, 2022 11:57
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 bed648c into nlohmann:develop Aug 28, 2022
@nlohmann
Copy link
Owner

Thanks a lot!!!

@barcode
Copy link
Contributor Author

barcode commented Aug 28, 2022

Thanks to you guys, too. I really like this library and know it is a lot of work to maintain libraries.

@@ -1200,11 +1204,12 @@ class basic_json // NOLINT(cppcoreguidelines-special-member-functions,hicpp-spec
/// @brief move constructor
/// @sa https://json.nlohmann.me/api/basic_json/basic_json/
basic_json(basic_json&& other) noexcept
: m_type(std::move(other.m_type)),
: json_base_class_t(std::move(other)),
Copy link
Owner

Choose a reason for hiding this comment

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

@barcode Sorry for being so late, but now I got a warning from Clang-Tidy about accessing other.m_value after moving other in the line before. Any idea how to fix this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation L review needed It would be great if someone could review the proposed changes. tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants