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

Basic nlohmann-json integration. #11967

Closed
wants to merge 14 commits into from
Closed

Basic nlohmann-json integration. #11967

wants to merge 14 commits into from

Conversation

axic
Copy link
Member

@axic axic commented Sep 15, 2021

Fixes #6900.

A partial rebase of aarlt@1329c25.

cmake/nlohmann-json.cmake Outdated Show resolved Hide resolved
@axic axic force-pushed the nlohmann-json branch 13 times, most recently from 6d9ca13 to 8b07ad3 Compare September 15, 2021 17:28
{
removeNullMembersHelper(_json);
// TODO: Support this.
Copy link
Member Author

Choose a reason for hiding this comment

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

This needs to be fixed.

if (_format.format == JsonFormat::Pretty)
boost::replace_all(result, " \n", "\n");
return result;
// TODO: support the other features
Copy link
Member Author

Choose a reason for hiding this comment

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

This needs to be fixed.

@axic axic force-pushed the nlohmann-json branch 2 times, most recently from c2528b5 to a7fc74b Compare September 15, 2021 17:49
@ethereum ethereum deleted a comment from stackenbotten Sep 15, 2021
@ethereum ethereum deleted a comment from stackenbotten Sep 15, 2021
@ethereum ethereum deleted a comment from stackenbotten Sep 15, 2021
@ethereum ethereum deleted a comment from stackenbotten Sep 15, 2021
@ethereum ethereum deleted a comment from stackenbotten Sep 15, 2021
@ethereum ethereum deleted a comment from stackenbotten Sep 15, 2021
@ethereum ethereum deleted a comment from stackenbotten Sep 15, 2021
@ethereum ethereum deleted a comment from stackenbotten Sep 15, 2021
@ethereum ethereum deleted a comment from stackenbotten Sep 15, 2021
@ethereum ethereum deleted a comment from stackenbotten Sep 15, 2021
@ethereum ethereum deleted a comment from stackenbotten Sep 15, 2021
@ethereum ethereum deleted a comment from stackenbotten Sep 15, 2021
@falbrechtskirchinger
Copy link

I guess this needs to be taken over?

I think this ran into some bug in one of the JSON output generators.

But also let's wait for nlohmann/json#3130 to be fixed. (Well was just merged now.)

Let me know if there're any more issues. I've not explicitly tested with range-v3 but std::ranges. I expect 3.11.0 to be released soon (although it's not up to me).

@Marenz
Copy link
Contributor

Marenz commented Sep 7, 2022

I pushed a rebase and will now look & go through the comments here

@axic
Copy link
Member Author

axic commented Sep 26, 2022

Rebased and upgraded to nlohmann-json 3.11.2 which has proper ranges-v3 support.

return _input.dump(
/* indent */ (_format.format == JsonFormat::Pretty) ? static_cast<int>(_format.indent) : -1,
/* indent_char */ ' ',
/* ensure_ascii */ true
Copy link
Member

Choose a reason for hiding this comment

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

should be false here - we have some tests using unicode

Copy link
Member Author

Choose a reason for hiding this comment

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

This means unicode stuff is escaped, which is the default behaviour of JSON expected by many implementations.

Copy link
Member

Choose a reason for hiding this comment

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

I just noticed that some tests are not working because of that. But yeah, we could also rewrite the tests - but maybe we should not.

@ekpyron
Copy link
Member

ekpyron commented Dec 1, 2022

Closing this as "postponed" for now. We'll keep the branch around as a basis for when we may pick this up again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
takeover Can be taken over by someone other than label giver
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider replacing jsoncpp
7 participants