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 issue#1989: only consider continuous numbers #2123

Closed
wants to merge 1 commit into from

Conversation

dota17
Copy link
Contributor

@dota17 dota17 commented May 18, 2020

In unflatten we just consider creating object, which means when we meet continuous numbers, we also unflatten to an object to represent the actual array. And then we convert those object with continuous numbers key to array.

#1989 (comment)

@dota17 dota17 requested a review from nlohmann as a code owner May 18, 2020 13:00
@coveralls
Copy link

Coverage Status

Coverage increased (+0.0001%) to 99.941% when pulling 00cbf8a on dota17:unflatten into 5cfa8a5 on nlohmann:develop.

@dota17
Copy link
Contributor Author

dota17 commented May 19, 2020

Please restart the job. It seems like a timeout error.

@dota17
Copy link
Contributor Author

dota17 commented Jun 1, 2020

Keep alive.
Is there any new review or idea about this?

@nlohmann
Copy link
Owner

nlohmann commented Jun 4, 2020

I am hesitant on this issue as it will break existing code in the sense that the unflattened values will differ.

@dota17
Copy link
Contributor Author

dota17 commented Jun 8, 2020

the unflattened values will differ.

The problem is that the different Json values can be flattened to the same result. Thus, we can not know which one is the origin one.
For the ambiguous cases, we should take a clear rule to decide which one is preferred.

it will break existing code

Maybe unflatten is a sick funtion.
If we don't break, the origin function will still throw an exception for some scenarios that should be supported. Maybe we can clear the doc or comment to explain the abnormal situation.
Or add an option to keep the origin code. #1575 (comment)

Honestly, I don't know why the feature is included and which reference functions it is. The infos may help a lot.

@stale
Copy link

stale bot commented Jul 11, 2020

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 Jul 11, 2020
@dota17 dota17 closed this Jul 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
state: stale the issue has not been updated in a while and will be closed automatically soon unless it is updated
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants