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

Fix nlohmann/json#3513, explain is_ndarray flag #3514

Merged
merged 2 commits into from
Jun 3, 2022

Conversation

fangq
Copy link
Contributor

@fangq fangq commented Jun 1, 2022

Sorry for keep creating these assertion errors. I hope this fix should seal it.

Essentially, the only line change needed to fix #3513 is to flip this boolean at the following line: NeuroJSON@3a65cc4#diff-a9407ccd45e5c9f004f9d81a18c381d0e5ecd782d5a91ec8f64614d3da6a7eaeR11247

however, I renamed the variable for better readability and added param comment to explain how is_ndarray flag is used as input and as output.

let me know if this looks ok to you.

@fangq fangq requested a review from nlohmann as a code owner June 1, 2022 21:14
@coveralls
Copy link

coveralls commented Jun 1, 2022

Coverage Status

Coverage remained the same at 100.0% when pulling 4c6cf3c on NeuroJSON:issue3513 into 6b97599 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.

Please add the input from #3513 as test case. I tried the new header, but still can reproduce the assertion.

@fangq
Copy link
Contributor Author

fangq commented Jun 3, 2022

test is now added.

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 Jun 3, 2022
@nlohmann nlohmann added this to the Release 3.11.0 milestone Jun 3, 2022
@nlohmann
Copy link
Owner

nlohmann commented Jun 3, 2022

Please add the input from #3513 as test case. I tried the new header, but still can reproduce the assertion.

My bad - I had an error in my test setup and copied your fixed header to the wrong place. The assertion is not triggered any more.

@nlohmann nlohmann merged commit 046927c into nlohmann:develop Jun 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ASSERT error while parsing BJData
3 participants