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): add params natspec for enums #10022

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

Conversation

samooyo
Copy link

@samooyo samooyo commented Mar 6, 2025

Motivation

Solution

The forge doc comment parser now supports parsing Enum variants in Natspecs and displaying them in a table. To declare these variants, you can use either the @param or @custom:variant tag.

To implement this feature:

  • Comments containing the @custom:variant tag are filtered out to prevent them from being displayed in the Note field.
  • Both the variant and param tags are parsed to properly display the table.

Additionally:

  • The From trait was implemented for the Comments struct to enable the construction of the returned vector from the filtered comments.
  • A dedicated table header and separator were added specifically for variants.

2
1

PR Checklist

  • Added Tests
  • Added Documentation
  • Breaking changes

Sorry, something went wrong.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
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.

Hi @samooyo, thanks for your PR!

Given that Solidity has not prioritised adding support for this in the current release I think it is reasonable to add this ahead of time and make adjustments where needed afterwards.

Would you mind adding a test case for this?

The proposed syntax of Solidity (ethereum/solidity#14193) is as follows:

    enum Color {
        Red,
        /// @notice example of notice
        /// @dev example of dev
        Green,
        /// @dev example of dev
        Blue
    }

Once Solidity has implement support for this in-line syntax we can expand it to also cover this case.

Personally I think the @param syntax as proposed makes most sense here

@@ -46,7 +46,7 @@ impl CommentTag {
}
_ => {
warn!(target: "forge::doc", tag=trimmed, "unknown comment tag. custom tags must be preceded by `custom:`");
return None
return None;
Copy link
Member

@zerosnacks zerosnacks Mar 7, 2025

Choose a reason for hiding this comment

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

nit, would you mind removing the inserted ; by cargo fmt in the PR, Foundry uses the nightly version (+nightly)

@zerosnacks zerosnacks added the T-to-discuss Type: requires discussion label Mar 7, 2025
…s#9905

Co-authored-by: gregorsternat <[email protected]>
Copy link
Contributor

@srdtrk srdtrk left a comment

Choose a reason for hiding this comment

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

Since it seems like CommentTag::Custom("variant".to_string()) is commonly used in this PR. Would it be ok to add the following method to CommentTag in parser.

    /// Create a new instance of [CommentTag::Custom] with the `variant` tag.
    pub fn variant() -> Self {
        Self::Custom("variant".to_string())
    }

On a side note @zerosnacks, I'm using the parser crate in an external project for a natspec linter I'm building (it's private at the moment) in my free time. I also have to maintain a copy of the parser module since it is not published on crates.io. Would you be open to refactor and publish the parser as a crate if I create an issue for this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-to-discuss Type: requires discussion
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

Added NatSpec support for enum value definitions
3 participants