Skip to content

Add operator< to json_pointer, to allow use as a map key #3667

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

Closed
wants to merge 0 commits into from
Closed

Add operator< to json_pointer, to allow use as a map key #3667

wants to merge 0 commits into from

Conversation

khiner
Copy link

@khiner khiner commented Aug 3, 2022

Hopefully, the commit description is self-explanatory.

This will require new unit tests, which I can write and add to this PR, but I wanted to get a sanity check that there's not some reason this is a bad idea.

Thank you!

@khiner khiner requested a review from nlohmann as a code owner August 3, 2022 20:16
@falbrechtskirchinger
Copy link
Contributor

FYI, there's another PR in flight, fixing a regression involving equality comparison (#3664). Seeing that the other operators are missing, I was planning on adding the rest eventually. This is a start.

@coveralls
Copy link

coveralls commented Aug 3, 2022

Coverage Status

Coverage remained the same at 100.0% when pulling 1909212 on khiner:develop into 22cd1c9 on nlohmann:develop.

@khiner
Copy link
Author

khiner commented Aug 4, 2022

I am having trouble finding the actual issue behind the failing amalgamation test. As far as I can tell, the indentation style matches the surrounding code exactly. Could someone more familiar with this test please help decipher? 🙏

@falbrechtskirchinger
Copy link
Contributor

single_include/nlohmann/json.hpp is missing. You need to run make amalgamate and add single_include/nlohmann/json.hpp to your commit. See https://github.com/nlohmann/json/blob/develop/.github/CONTRIBUTING.md#files-to-change.

@khiner
Copy link
Author

khiner commented Aug 4, 2022

Ah! Sorry I missed that 🤦‍♂️

@falbrechtskirchinger
Copy link
Contributor

falbrechtskirchinger commented Aug 4, 2022

This still looks off. Did you let your editor reformat any of the files?

We use Artistic Style which does not create reproducible formatting. I.e., differently formatted input may produce different output.

Edit: Specifically, json_fwd.hpp and json_ref.hpp.

@khiner
Copy link
Author

khiner commented Aug 4, 2022

Hrm, there are no changed files in my lib/json directory. The only changes from develop whatsoever were the json_pointer.hpp changes in this PR. I exclude lib/json/** from formatting explicitly in my IDE settings. Maybe something about my environment caused make amalgamate to run differently? Maybe you could try running with just my non-amalgamate changes?

@falbrechtskirchinger
Copy link
Contributor

Fixed it for you. https://github.com/khiner/json/pull/1/

@khiner
Copy link
Author

khiner commented Aug 5, 2022

Thanks @falbrechtskirchinger ! I rebased that on my branch. Still not sure why the run output is different between us 🤷‍♂️

@khiner
Copy link
Author

khiner commented Aug 5, 2022

ayy looks like the other equality operators PR got in, so I'll have to rework this one

@falbrechtskirchinger
Copy link
Contributor

We're moving fast right now because we want to get v3.11.2 out the door ASAP.

FYI, my Artistic Style version is:

$ astyle --version
Artistic Style Version 3.1

Don't know if that might be causing the difference.

@github-actions github-actions bot added the M label Aug 5, 2022
@khiner khiner closed this Aug 5, 2022
@khiner
Copy link
Author

khiner commented Aug 5, 2022

Turns out I didn't have astyle installed at all, heh. So that explains that. Doesn't seem like it should run without warning/error in that case though. Either way, after installing, my amalgamate doesn't mess with other lines I didn't touch.

I'm opening up a new PR based off current develop

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants