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): inspect - default to pretty output #9705

Merged
merged 18 commits into from
Jan 23, 2025
Merged

Conversation

yash-atreya
Copy link
Member

@yash-atreya yash-atreya commented Jan 17, 2025

Motivation

Closes #5165

Solution

  • Removes the --pretty flag and makes that behavior the default.
  • Generate table layout for abi, method-identifiers, errors, and events by default, json can be obtained using the --json.
  • For artifacts fields that are not table-friendly, generate json by default.

@yash-atreya yash-atreya self-assigned this Jan 20, 2025
@yash-atreya yash-atreya marked this pull request as ready for review January 20, 2025 07:10
@yash-atreya yash-atreya added C-forge Command: forge T-feature Type: feature labels Jan 20, 2025
@yash-atreya yash-atreya changed the title fix(forge): inspect - default to pretty output feat(forge): inspect - default to pretty output Jan 20, 2025
@yash-atreya yash-atreya added the T-likely-breaking Type: requires changes that can be breaking label Jan 20, 2025
@grandizzy
Copy link
Collaborator

per comment #5165 (comment) I think we want to totally remove the --pretty flag (and rely only on shell::is_json()) and have table applied for abi too, that is instead an output like below to be printed a table

forge inspect src/Counter.sol:Counter abi
interface Counter {
    error MyError();

    event MyEvent();

    function increment() external;
    function number() external view returns (uint256);
    function setNumber(uint256 newNumber) external;
}

hope I got this right, @zerosnacks pls correct me if I am wrong

crates/forge/bin/cmd/inspect.rs Outdated Show resolved Hide resolved
crates/forge/bin/cmd/inspect.rs Outdated Show resolved Hide resolved
@yash-atreya yash-atreya enabled auto-merge (squash) January 20, 2025 08:42
@yash-atreya yash-atreya disabled auto-merge January 20, 2025 08:42
@zerosnacks
Copy link
Member

per comment #5165 (comment) I think we want to totally remove the --pretty flag (and rely only on shell::is_json()) and have table applied for abi too, that is instead an output like below to be printed a table

forge inspect src/Counter.sol:Counter abi
interface Counter {
    error MyError();

    event MyEvent();

    function increment() external;
    function number() external view returns (uint256);
    function setNumber(uint256 newNumber) external;
}

hope I got this right, @zerosnacks pls correct me if I am wrong

Agreed, we want to optimize the default output for human readability and if users want to use the output in a script they should rely on the --json flag

@@ -29,18 +30,14 @@ pub struct InspectArgs {
#[arg(value_enum)]
pub field: ContractArtifactField,

/// Pretty print the selected field, if supported.
#[arg(long)]
pub pretty: bool,
Copy link
Member

Choose a reason for hiding this comment

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

we should keep this with a warning or document as a breaking change

Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah, will be marked in release notes as breaking change given the breaking label

@yash-atreya yash-atreya marked this pull request as draft January 20, 2025 13:49
@zerosnacks
Copy link
Member

zerosnacks commented Jan 20, 2025

for reference: cast interface <CONTRACT> also works on local artifacts for users that do look for a way to generate an interface

@yash-atreya yash-atreya marked this pull request as ready for review January 21, 2025 09:02
@yash-atreya
Copy link
Member Author

@grandizzy @zerosnacks ptal

Copy link
Member

@zerosnacks zerosnacks left a comment

Choose a reason for hiding this comment

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

generally lgtm, note on additional tests + minor style nit

in the release note we can mention the cast interface <CONTRACT> equivalent

crates/forge/bin/cmd/inspect.rs Outdated Show resolved Hide resolved
crates/forge/bin/cmd/inspect.rs Show resolved Hide resolved
Copy link
Collaborator

@grandizzy grandizzy left a comment

Choose a reason for hiding this comment

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

lgtm, pending @zerosnacks review

@zerosnacks
Copy link
Member

lgtm!

@yash-atreya yash-atreya merged commit c22c4cc into master Jan 23, 2025
22 checks passed
@yash-atreya yash-atreya deleted the yash/fix-5165 branch January 23, 2025 08:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-forge Command: forge T-feature Type: feature T-likely-breaking Type: requires changes that can be breaking
Projects
Status: Completed
Development

Successfully merging this pull request may close these issues.

Add table layout for forge inspect <contract> abi
4 participants