Skip to content

Conversation

@LocutusOfBorg
Copy link
Contributor

@LocutusOfBorg LocutusOfBorg commented Jul 22, 2025

As said, I would like to package test_data jsons in Debian...

@coveralls
Copy link

coveralls commented Jul 22, 2025

Coverage Status

coverage: 99.191%. remained the same
when pulling c0476f3 on LocutusOfBorg:develop
into 8deac49 on nlohmann:develop.

@github-actions
Copy link

This pull request has been marked as stale because it has had no activity for 30 days. While we won’t close it automatically, we encourage you to update or comment if it is still relevant. Keeping pull requests active and up-to-date helps us review and merge changes more efficiently. Thank you for your contributions!

@github-actions github-actions 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 Aug 22, 2025
@LocutusOfBorg
Copy link
Contributor Author

rebased

@github-actions github-actions bot removed the state: stale the issue has not been updated in a while and will be closed automatically soon unless it is updated label Aug 26, 2025
@LocutusOfBorg
Copy link
Contributor Author

json-test-data is now part of sid, and I removed the hardcoded version from the directory. @nlohmann can you please have another look? thanks

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.

I think the idea of the PR can be achieved by adding test labels and leaving the CMake files as is. Then a test directory can be passed via -DJSON_TEST_DATA_DIR=... and tests that require internet can be excluded via ctest -LE ....

@LocutusOfBorg LocutusOfBorg force-pushed the develop branch 2 times, most recently from 0acd9c8 to 4099eed Compare October 25, 2025 07:28
@github-actions github-actions bot added S and removed M labels Oct 25, 2025
@LocutusOfBorg
Copy link
Contributor Author

So nothing left to merge :D

@nlohmann
Copy link
Owner

No, I think you still need to annotate the tests that require internet so you can skip those.

@LocutusOfBorg
Copy link
Contributor Author

@nlohmann the tests requiring git are the same requiring internet :)
Maybe git_required should be changed into network_required, because they need git to clone from network, but looks too much to add another label just for the same purpose?

@nlohmann
Copy link
Owner

I'd rather not add an alias label. However, maybe this text from the README could be updated:

In case you have downloaded the library rather than checked out the code via Git, test cmake_fetch_content_configure will fail. Please execute ctest -LE git_required to skip these tests. See #2189 for more information.

Thoughts?

@LocutusOfBorg LocutusOfBorg reopened this Oct 30, 2025
@LocutusOfBorg LocutusOfBorg force-pushed the develop branch 2 times, most recently from 1a090a9 to 3eecdbc Compare October 30, 2025 09:44
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 revert this change. Renaming the label will be breaking other people's workflows.

@github-actions github-actions bot added documentation and removed S labels Oct 30, 2025
@LocutusOfBorg
Copy link
Contributor Author

What do you think about this one?

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.

No, please do not add another label. This is all complexity down the road. I think a note to the README that if somebody wants to execute the test suite without internet, then git_required is there to help.

(Sorry for being so insistent, but this is a side project, and I have to be careful of adding stuff.)

@LocutusOfBorg
Copy link
Contributor Author

ack done

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.12.1 milestone Nov 2, 2025
@nlohmann nlohmann merged commit 0c9b68e into nlohmann:develop Nov 2, 2025
142 checks passed
@nlohmann
Copy link
Owner

nlohmann commented Nov 2, 2025

Thanks!

@LocutusOfBorg
Copy link
Contributor Author

thanks to you!

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.

3 participants