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

feat: Rebase feature/optional to develop #4036

Merged
merged 16 commits into from
Nov 16, 2024

Conversation

fsandhei
Copy link
Contributor

@fsandhei fsandhei commented May 20, 2023

Rebasing feature/optional in attempt to revive this PR, referring to #2117.

Sorry for long time between responses.

I may have been going forward with this the "wrong" way, so I apologize in advance.

Please let me know if this should be handled differently.

TODO

There is one test that is failing: Conversions from a default initialized nlohmann::json to std::optional<T> seems to fail. In the test case below, the last assertion fails with an exception. This is tested on Arch Linux with GCC 13.1.1.

I was not able to figure out how to fix this so I need some assistance here.

/home/fredrik/dev/github/cpp/fredriksd/json/tests/src/unit-conversions.cpp:1573:
TEST CASE:  std::optional
  null

/home/fredrik/dev/github/cpp/fredriksd/json/tests/src/unit-conversions.cpp:1573: ERROR: test case THREW exception: exception thrown in subcase - will translate later when the whole test case has been exited (cannot translate while there is an active exception)

===============================================================================
/home/fredrik/dev/github/cpp/fredriksd/json/tests/src/unit-conversions.cpp:1573:
TEST CASE:  std::optional

DEEPEST SUBCASE STACK REACHED (DIFFERENT FROM THE CURRENT ONE):
  null

/home/fredrik/dev/github/cpp/fredriksd/json/tests/src/unit-conversions.cpp:1573: ERROR: test case THREW exception: [json.exception.type_error.302] type must be string, but is null

@fsandhei fsandhei requested a review from nlohmann as a code owner May 20, 2023 10:12
@fsandhei fsandhei changed the title Feature/optional feat: Rebase feature/optional to develop May 20, 2023
@fsandhei fsandhei closed this May 20, 2023
@fsandhei fsandhei reopened this May 20, 2023
@fsandhei fsandhei changed the base branch from feature/optional to develop May 20, 2023 10:43
@fsandhei
Copy link
Contributor Author

Changed the base branch to develop.

I'm sorry if I'm making it a bit difficult here; I forked the repository and rebased feature/optional to develop and created this PR against nlohmann:feature/develop but I noticed that included 590 commits (which makes sense) but it cluttered the actual change here, so I instead changed it towards nlohmann:develop.

@lightyear15
Copy link

lightyear15 commented Feb 27, 2024

I am also interested in having std::optional support landing in future versions of this library.
I see there was a lot of discussion on how to properly support and convert c++ optional fields vs json nullable fields.
Is there a final decision on the matter? How can we revive this PR and push forward support of std::optional fields?
@nlohmann

@coveralls
Copy link

Coverage Status

coverage: 100.0%. remained the same
when pulling 1a8d427 on fsandhei:feature/optional
into 199dea1 on nlohmann:develop.

@fsandhei
Copy link
Contributor Author

fsandhei commented Apr 6, 2024

See that clang-tidy is failing on some checks. Apparently this is on develop, too. I see that it is related to a supposed new check from clang-tidy v19: https://clang.llvm.org/extra/clang-tidy/checks/modernize/use-designated-initializers.html.

@nlohmann
Copy link
Owner

nlohmann commented Apr 8, 2024

See that clang-tidy is failing on some checks. Apparently this is on develop, too. I see that it is related to a supposed new check from clang-tidy v19: clang.llvm.org/extra/clang-tidy/checks/modernize/use-designated-initializers.html.

I'm on it. May take some more time.

@lightyear15
Copy link

has this PR stalled again?

@fsandhei
Copy link
Contributor Author

fsandhei commented May 8, 2024

I'm currently blocked by #4311.

@fsandhei
Copy link
Contributor Author

There has not been any activity in #4311 in a while. I wonder if this would be okay to merge?

@nlohmann
Copy link
Owner

Thanks for keeping this alive. I will have a look at this on Saturday.

@coveralls
Copy link

coveralls commented Nov 14, 2024

Coverage Status

coverage: 100.0%. remained the same
when pulling 296549c on fsandhei:feature/optional
into 1825117 on nlohmann:develop.

@fsandhei
Copy link
Contributor Author

Thanks for keeping this alive. I will have a look at this on Saturday.

Happy to help! Hopefully this can go in soon.

@nlohmann
Copy link
Owner

Please suppress the last clang-tidy warning and not change the code.

@fsandhei
Copy link
Contributor Author

Please suppress the last clang-tidy warning and not change the code.

Reverted change with 296549c.

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 added this to the Release 3.11.4 milestone Nov 16, 2024
@nlohmann nlohmann merged commit 0604140 into nlohmann:develop Nov 16, 2024
124 checks passed
@nlohmann
Copy link
Owner

Thanks!

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.

[C++17] Allow std::optional to convert to nlohmann::json
5 participants