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

Add assertion to converting constructor #3517

Merged

Conversation

falbrechtskirchinger
Copy link
Contributor

@falbrechtskirchinger falbrechtskirchinger commented Jun 3, 2022

The converting basic_json constructor can inadvertently change the value type of its parameter. Assert that both basic_json values are of the same value type after conversion.

This is not a fix for #3425, but instead of surprising the user with strings converting to arrays, etc., those conversions will at least fail.

The converting basic_json constructor can inadvertently change the value
type of its parameter. Assert that both basic_json values are of the
same value type after conversion.
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 4b75ae6 on falbrechtskirchinger:assert-conv-value_t into 6058d9a on nlohmann:develop.

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

nlohmann commented Jun 3, 2022

Please change the wording about fixing: GitHub does not understand the NOT and would close #3425 when merging.

@falbrechtskirchinger
Copy link
Contributor Author

falbrechtskirchinger commented Jun 3, 2022

@nlohmann Done.

Edit: But you might want to wait. I think I'm close to a proper solution to #3425 that's quite a bit simpler than my previous attempt. 🤞

@nlohmann nlohmann self-assigned this Jun 3, 2022
@nlohmann nlohmann added this to the Release 3.11.0 milestone Jun 3, 2022
@nlohmann nlohmann merged commit 7a6e28a into nlohmann:develop Jun 3, 2022
@falbrechtskirchinger falbrechtskirchinger deleted the assert-conv-value_t branch June 3, 2022 19:06
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

4 participants