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(forge): added --json argument to forge build command #6465

Merged
merged 7 commits into from
Dec 1, 2023
Merged

feat(forge): added --json argument to forge build command #6465

merged 7 commits into from
Dec 1, 2023

Conversation

0xmemorygrinder
Copy link
Contributor

This PR adds a --json argument to forge compile command to output the compilation warnings/errors in json

Motivation

Currently integrating foundry in VSCode, my team and I quickly realized there is no output format that can be easily parsed for the forge compile command. The goal is thus to add a json to output the compilaton result in the JSON format.

Solution

To implement this feature, I added a json field in the BuildArgs structure and added a condition in the run method of the command to check if the argument has been used.

If so, a new function suppress_compile_with_filter_json is called. It behaves like the suppress_compile_with_filter function but without throwing error when compilation errors arise.

All the other modified files are due to the new function parameters which I added to indicate to the function whether to throw or not.

Additional comments

I not sure if I should have add the json argument in the BuildArgs structure or in the CoreBuildArgs structure. Let me know if I should fix this.

This PR does not contain test for the new feature as I only rely on serde_json and there was no tests for the compile command beforehand.

Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

supportive, I can see how this is useful.

smol nits

or in the CoreBuildArgs structure

the new flag should stay in BuildArgs, because it's build command specific. other commands also have a json flag, like test which would then conflict if we have it in CoreBuildArgs

we also want a test for this:

add new tests/it/build.rs file, add contract that has an error, run build command, check output.

ref example test you can use as base

// tests that warning is displayed with pattern when no tests match
forgetest!(warn_no_tests_match, |prj, cmd| {
prj.add_source(
"dummy",
r"
contract Dummy {}
",
)
.unwrap();
// set up command
cmd.args(["test", "--match-test", "testA.*", "--no-match-test", "testB.*"]);
cmd.args(["--match-contract", "TestC.*", "--no-match-contract", "TestD.*"]);
cmd.args(["--match-path", "*TestE*", "--no-match-path", "*TestF*"]);
// run command and assert
cmd.unchecked_output().stdout_matches_path(
PathBuf::from(env!("CARGO_MANIFEST_DIR")).join("tests/fixtures/warn_no_tests_match.stdout"),
);
});

you can generate fixtures via cmd.write_fixtures(path) and then use it

crates/forge/bin/cmd/build.rs Outdated Show resolved Hide resolved
crates/forge/bin/cmd/build.rs Outdated Show resolved Hide resolved
crates/common/src/compile.rs Outdated Show resolved Hide resolved
@0xmemorygrinder
Copy link
Contributor Author

Thank you for your valuable feedback @mattsse. I implemented all your requests. Let me know if I can do something else

Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

this looks great!

I'd like to see a simple test case for json output, see instructions above

@0xmemorygrinder
Copy link
Contributor Author

Does the one in crates/forge/test/cli/build.rs not fulfill your requirements ?
I implemented it following your above advice.

Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

my bad, I must have overlooked this, hehe

@mattsse mattsse merged commit 8b7500b into foundry-rs:master Dec 1, 2023
19 checks passed
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