json icon indicating copy to clipboard operation
json copied to clipboard

feat: Rebase `feature/optional` to `develop`

Open fsandhei opened this issue 2 years ago • 8 comments

Rebasing feature/optional in attempt to revive this PR, referring to https://github.com/nlohmann/json/pull/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 avatar May 20 '23 10:05 fsandhei

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.

fsandhei avatar May 20 '23 10:05 fsandhei

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

lightyear15 avatar Feb 27 '24 11:02 lightyear15

Coverage Status

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

coveralls avatar Mar 26 '24 16:03 coveralls

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.

fsandhei avatar Apr 06 '24 18:04 fsandhei

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.

nlohmann avatar Apr 08 '24 19:04 nlohmann

has this PR stalled again?

lightyear15 avatar May 08 '24 08:05 lightyear15

I'm currently blocked by https://github.com/nlohmann/json/pull/4311.

fsandhei avatar May 08 '24 15:05 fsandhei

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

fsandhei avatar Aug 12 '24 10:08 fsandhei