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

Add jsoncpp_test bazel configuration #1599

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Add jsoncpp_test bazel configuration #1599

wants to merge 2 commits into from

Conversation

baylesj
Copy link
Contributor

@baylesj baylesj commented Mar 12, 2025

This patch adds a new configuration for jsoncpp_test in the BUILD.bazel file.

baylesj added 2 commits March 12, 2025 16:12
This patch adds a new configuration for jsoncpp_test in
the BUILD.bazel file.
@coveralls
Copy link

coveralls commented Mar 12, 2025

Pull Request Test Coverage Report for Build 13823356124

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 95.292%

Totals Coverage Status
Change from base Build 13823123241: 0.0%
Covered Lines: 5364
Relevant Lines: 5629

💛 - Coveralls

@@ -22,10 +22,6 @@ cc_library(
"include/json/version.h",
"include/json/writer.h",
],
copts = [
"-DJSON_USE_EXCEPTION=0",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing this could be a breaking change for some users. I think a better way to do this would be: #1600

@@ -35,3 +31,22 @@ cc_library(
name = "private",
textual_hdrs = ["src/lib_json/json_valueiterator.inl"],
)

cc_binary(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both of these should likely be testonly=True and should probably also be in BUILD.bazel files in their respective directories.

deps = [":jsoncpp"],
)

cc_binary(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this could just be made to be a cc_test rule. ...

And I got a little carried away check if that was true: #1601

(That said, I'm not 100% happy with how that came out if you want to take a run at making it cleaner.)

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