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.

Sign up for GitHub

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

Jump to bottom

Amino proto file loses Go doc comments #1157

Closed
jefft0 opened this issue Sep 20, 2023 · 3 comments
Closed

Amino proto file loses Go doc comments #1157

jefft0 opened this issue Sep 20, 2023 · 3 comments
Milestone
💡Someday/Maybe

Comments

@jefft0
Copy link
Contributor

jefft0 commented Sep 20, 2023 •
edited
Loading

Consider this Go struct where the Receiver field has a comment. Amino converts it but the proto message Receiver field doesn't have the comment.

I know that Amino uses reflection of the Go struct type which doesn't include comments. But we could use parser.ParseFile to get the AST from the source code file, only for the purpose of finding comments. Then amino could copy the doc comment into the proto message.

What do you think? Is it desirable for the proto message output from amino to include field comments? Is there another way to do it?
This was referenced Sep 20, 2023
Open
Closed
@moul
Copy link
Member

moul commented Sep 20, 2023

I definitely agree with the idea of forwarding comments to protobuf. However, I'm not in favor of having amino files with comments that are intended to become protobuf-specific behaviors. Instead, I suggest adding new helpers so we can annotate amino or maybe comment in a way that makes sense generally, not just for protobuf.

I'm not opposed to having a dedicated ProtobufExtra helper. This could be explicit enough and easily skipped by anyone not interested in protobuf-specific details. This way, we maintain the general applicability of our comments and annotations, while also providing the option for more protobuf-specific details for those who need it.

@moul moul moved this to 🔵 Not Needed for Launch in 🚀 The Launch [DEPRECATED] Sep 20, 2023
@moul moul added this to the 💡Someday/Maybe milestone Sep 20, 2023
@jefft0
Copy link
Contributor Author

jefft0 commented Sep 21, 2023

It looks like amino has support for field comments in the P3Field struct. But I don't see any code which sets it (other than tests).

@jefft0 jefft0 changed the title Amino proto file loses Go doc comments. Put doc comment in a tag? Amino proto file loses Go doc comments Sep 21, 2023
@jefft0 jefft0 mentioned this issue Sep 21, 2023
Open
@jefft0 jefft0 mentioned this issue Oct 11, 2023
Merged
7 tasks
thehowl added a commit that referenced this issue Nov 23, 2023
@jefft0 @thehowl
chore: In amino.pkg, add optional WithComments, use them in GenerateP…
d421b96 
…roto3MessagePartial (#1235)

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;
    }

<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 <[email protected]>
Co-authored-by: Morgan Bazalgette <[email protected]>
@jefft0
Copy link
Contributor Author

jefft0 commented Nov 23, 2023

Closing as resolved by PR #1235 .

@jefft0 jefft0 closed this as completed Nov 23, 2023
gfanton pushed a commit to moul/gno that referenced this issue Jan 18, 2024
@jefft0 @thehowl @gfanton
chore: In amino.pkg, add optional WithComments, use them in GenerateP…
f44bc99 
…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 <[email protected]>
Co-authored-by: Morgan Bazalgette <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
🚀 The Launch [DEPRECATED]
Status: 🔵 Not Needed for Launch
Milestone
💡Someday/Maybe
Development

No branches or pull requests
2 participants
@moul @jefft0