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

chore: In amino.pkg, add optional WithComments, use them in GenerateProto3MessagePartial #1235

Merged
merged 4 commits into from
Nov 23, 2023

Conversation

jefft0
Copy link
Contributor

@jefft0 jefft0 commented Oct 11, 2023

This PR resolves issue #1157 by adding support for the optional WithComments. See the issue for motivation.

  • In amino.pkg.Type, add optional fields Comment and FieldComments .
  • Add the Package method WithComments which can optionally be used after WithTypes. This reads the Go source file and scans the AST for struct and field comments which are added to the Type object.
  • Update GenerateProto3MessagePartial to copy the comments to the P3Doc and related P3Field objects which already have an optional Comment field for comments that are already included in the Protobuf file.
  • Add test file comments_test.go .

As shown in the test, you can use WithComments after WithTypes:

pkg := amino.RegisterPackage(
    amino.NewPackage(
        "github.com/gnolang/gno/tm2/pkg/amino/genproto",
        "amino_test",
        amino.GetCallersDirname(),
    ).WithTypes(
        &TestMessageName{},
        &TestMessageName2{},
    // Add comments from this same source file.
    ).WithComments(path.Join(amino.GetCallersDirname(), "comments_test.go")))

If your Go struct looks like:

// message comment
type TestMessageName struct {
    // field comment 1
    FieldName1 string
    // field comment 2
    FieldName2 []uint64
}

then your Protobuf file has:

// message comment
message TestMessageName {
    // field comment 1
    string FieldName1 = 1;
    // field comment 2
    repeated uint64 FieldName2 = 2;
}
Contributors' checklist...
  • Added new tests, or not needed, or not feasible
  • Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory
  • Updated the official documentation or not needed
  • No breaking changes were made, or a BREAKING CHANGE: xxx message was included in the description
  • Added references to related issues and PRs
  • Provided any useful hints for running manual tests
  • Added new benchmarks to generated graphs, if any. More info here.

@codecov
Copy link

codecov bot commented Oct 11, 2023

Codecov Report

Attention: 39 lines in your changes are missing coverage. Please review.

Comparison is base (5aa8676) 55.59% compared to head (b746716) 55.84%.
Report is 6 commits behind head on master.

Files Patch % Lines
tm2/pkg/amino/pkg/pkg.go 0.00% 39 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1235      +/-   ##
==========================================
+ Coverage   55.59%   55.84%   +0.25%     
==========================================
  Files         420      422       +2     
  Lines       65470    65686     +216     
==========================================
+ Hits        36395    36685     +290     
+ Misses      26222    26130      -92     
- Partials     2853     2871      +18     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…roto3MessagePartial

Signed-off-by: Jeff Thompson <jeff@thefirst.org>
Copy link
Member

@thehowl thehowl left a comment

Choose a reason for hiding this comment

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

LGTM

tm2/pkg/amino/pkg/pkg.go Outdated Show resolved Hide resolved
Copy link
Member

@gfanton gfanton left a comment

Choose a reason for hiding this comment

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

lgtm (after flatten :) )

thehowl and others added 2 commits November 23, 2023 18:53
… rinfo.Package .

Signed-off-by: Jeff Thompson <jeff@thefirst.org>
@jefft0
Copy link
Contributor Author

jefft0 commented Nov 23, 2023

Hi @thehowl . Thanks for doing the flattening. I pushed one last commit to fix a problem with running make in misc/genproto.
b746716

@thehowl thehowl merged commit d421b96 into gnolang:master Nov 23, 2023
178 of 179 checks passed
gfanton pushed a commit to moul/gno that referenced this pull request Jan 18, 2024
…roto3MessagePartial (gnolang#1235)

This PR resolves issue gnolang#1157 by adding support for the optional
`WithComments`. See the issue for motivation.

- In `amino.pkg.Type`, add optional fields `Comment` and `
FieldComments` .
- Add the `Package` method `WithComments` which can optionally be used
after `WithTypes`. This reads the Go source file and scans the AST for
struct and field comments which are added to the `Type` object.
- Update `GenerateProto3MessagePartial` to copy the comments to the
`P3Doc` and related `P3Field` objects which already have an optional
`Comment` field for comments that are already included in the Protobuf
file.
- Add test file `comments_test.go` .

As shown in the test, you can use `WithComments` after `WithTypes`:

    pkg := amino.RegisterPackage(
        amino.NewPackage(
            "github.com/gnolang/gno/tm2/pkg/amino/genproto",
            "amino_test",
            amino.GetCallersDirname(),
        ).WithTypes(
            &TestMessageName{},
            &TestMessageName2{},
        // Add comments from this same source file.
).WithComments(path.Join(amino.GetCallersDirname(),
"comments_test.go")))

If your Go struct looks like:

    // message comment
    type TestMessageName struct {
        // field comment 1
        FieldName1 string
        // field comment 2
        FieldName2 []uint64
    }

then your Protobuf file has:

    // message comment
    message TestMessageName {
        // field comment 1
        string FieldName1 = 1;
        // field comment 2
        repeated uint64 FieldName2 = 2;
    }

<details><summary>Contributors' checklist...</summary>

- [x] Added new tests, or not needed, or not feasible
- [x] Provided an example (e.g. screenshot) to aid review or the PR is
self-explanatory
- [x] Updated the official documentation or not needed
- [x] No breaking changes were made, or a `BREAKING CHANGE: xxx` message
was included in the description
- [x] Added references to related issues and PRs
- [x] Provided any useful hints for running manual tests
- [ ] Added new benchmarks to [generated
graphs](https://gnoland.github.io/benchmarks), if any. More info
[here](https://github.com/gnolang/gno/blob/master/.benchmarks/README.md).
</details>

---------

Signed-off-by: Jeff Thompson <jeff@thefirst.org>
Co-authored-by: Morgan Bazalgette <morgan@morganbaz.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📦 🌐 tendermint v2 Issues or PRs tm2 related
Projects
Status: Done
Status: 🔵 Not Needed for Launch
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants