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: Add "Since:" on proto doc comments #10434

Merged
merged 7 commits into from Oct 27, 2021
Merged

chore: Add "Since:" on proto doc comments #10434

merged 7 commits into from Oct 27, 2021

Conversation

amaury1093
Copy link
Contributor

Description

ref: #10406 (reply in thread)

For clients to know whether a protobuf feature is available for a certain SDK version, we decided to use the @since doc comment inside protobuf files.


Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • included the correct type prefix in the PR title
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the guidelines for building modules
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed ! in the type prefix if API or client breaking change
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic
  • reviewed API design and naming
  • reviewed documentation is accurate
  • reviewed tests and test coverage
  • manually tested (if applicable)

@codecov
Copy link

codecov bot commented Oct 25, 2021

Codecov Report

Merging #10434 (42d543e) into master (b565069) will increase coverage by 0.03%.
The diff coverage is n/a.

❗ Current head 42d543e differs from pull request most recent head 83a30b8. Consider uploading reports for the commit 83a30b8 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master   #10434      +/-   ##
==========================================
+ Coverage   64.16%   64.19%   +0.03%     
==========================================
  Files         575      575              
  Lines       54923    54923              
==========================================
+ Hits        35239    35256      +17     
+ Misses      17682    17664      -18     
- Partials     2002     2003       +1     
Impacted Files Coverage Δ
crypto/keys/internal/ecdsa/privkey.go 82.45% <0.00%> (-1.76%) ⬇️
x/distribution/simulation/operations.go 90.27% <0.00%> (+9.72%) ⬆️

@@ -32,6 +32,8 @@ message PageRequest {
bool count_total = 4;

// reverse is set to true if results are to be returned in the descending order.
//
// @since Cosmos SDK 0.43
Copy link
Contributor Author

Choose a reason for hiding this comment

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

other ideas that came up:

  • @since cosmos-sdk 0.43
  • @since github.com/cosmos/cosmos-sdk@v0.43 (but that's not actually a valid github or go module tag)
  • @since 0.43

Opinions?

Copy link
Member

Choose a reason for hiding this comment

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

Turns out there is a bit of a collision then using the JSDoc/TSDoc style @since. It is represented as if it's about the JS/TS library version. See https://jsdoc.app/tags-since.html The same problem would apply for types generated in Java.

Bildschirmfoto 2021-10-25 um 23 13 56

I think it's better to avoid this conflict and just make it plain text for the callers, starting with Since:.

Since: cosmos-sdk 0.43 would be my favourite. It is clean, refers to the repo name and allows us to split by whitespace when parsing.

Copy link
Member

Choose a reason for hiding this comment

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

I think that's actually okay if we stick to strict API based versioning - the npm or maven package version would line up with the go version. I know want to have a high level user friendly package. But for individual low level proto packages I think could make a lot of sense to have own npm package per proto package with the exact same API version. Then the @since tag would work nicely with existing tools.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that's actually okay if we stick to strict API based versioning - the npm or maven package version would line up with the go version

Personally I actually don't see a lot of advantages for having the npm/maven/go/proto types versions all be the same. On the other hand, one disadvantage I see is the need to lock proto bumping speed with JS/Java/Rust bumping speed.

Simon mentioned yesterday about CosmJS using the long library for big integers. If CosmJS decides to change that to another lib (e.g. the native TypeScript bigint) which is a breaking change, they would need to wait for the proto pkg version to bump... which by design would take a long time.

Copy link
Member

Choose a reason for hiding this comment

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

While I agree with the idea of module specific packes that are close to Cosmos SDK versioning, enforcing the same version number across different client ecosystems seems to be very restricting. There can be breaking changes in the code generation that are not related to the API itself as Amury described. Some ecosystems structure in smaller packages, some in larger. Also note that the code generation does not necessarily happen in a type library. You can always add types in the application. Then you suddenly bind Cosmos SDK version to the version of the app.

Copy link
Member

@webmaster128 webmaster128 left a comment

Choose a reason for hiding this comment

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

Looks good to me. Do you mind backporting this to 0.44?

@@ -11,6 +11,8 @@ option (gogoproto.goproto_getters_all) = false;

// GenericAuthorization gives the grantee unrestricted permissions to execute
// the provided method on behalf of the granter's account.
//
// Since: cosmos-sdk 0.43
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For authz, I added this comment on all messages, and on the Query and Msg services.

Would it be possible to add it on the top-level package only?

syntax = "proto3";
// Since: cosmos-sdk 0.43
package cosmos.authz.v1beta1;

(note: that's what I did for cosmos.base.reflection.v2alpha1 for now, because there's a lot of stuff inside that file).

Copy link
Member

Choose a reason for hiding this comment

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

Good question. I agree there should be package-level documentation. However, there does not seem to be a point in the *.proto definitions where a package is created. It's created implicitly once you assign messages to it and as a consequence this is spread across multiple files.

In ts-proto it looks like file-level comments in the very top are copied to the .ts output, but later comments and package comments are not:

--- a/proto/cosmos/auth/v1beta1/auth.proto
+++ b/proto/cosmos/auth/v1beta1/auth.proto
@@ -1,4 +1,9 @@
+// Foo
 syntax = "proto3";
+
+// Bar
+
+// Since: cosmos-sdk 0.42
 package cosmos.auth.v1beta1;
 
 import "cosmos_proto/cosmos.proto";

becomes

--- a/src/cosmos/auth/v1beta1/auth.ts
+++ b/src/cosmos/auth/v1beta1/auth.ts
@@ -5,6 +5,8 @@ import { Any } from "../../../google/protobuf/any";
 
 export const protobufPackage = "cosmos.auth.v1beta1";
 
+/** Foo */
+
 /**
  * BaseAccount defines a base account type. It contains all the necessary fields
  * for basic accou

So what about putting it in the top of the file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For a dev that imports CosmJS, would their IDE intellisense show "Foo" somewhere?

It would be ideal if that comment showed up everytime they typed cosmos.authz{...}

Copy link
Member

Choose a reason for hiding this comment

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

For a dev that imports CosmJS, would their IDE intellisense show "Foo" somewhere?

No, but at least its visible when you dig into it or have it available for support.

It would be ideal if that comment showed up everytime they typed cosmos.authz{...}

Unfortunately we don't get the proto packages bundled into a common object. They are imported by file like this. See also cosmos/cosmjs#647.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Personally I also find it more elegant to put it in the top of the file. I did that in f74fd81.

ts-proto outputs the comments just fine, it would be great to get an idea on other languages (e.g. Rust) too. Duplicating those comments is cumbersome, but also safer (will be outputed on intellisense0. Any thoughts @aaronc ?

@amaury1093 amaury1093 changed the title chore: Add @since on proto doc comments chore: Add "Since:" on proto doc comments Oct 26, 2021
@amaury1093
Copy link
Contributor Author

amaury1093 commented Oct 26, 2021

R4R. Hopefully I didn't forget any place (I used git diff on .proto files)

@amaury1093 amaury1093 marked this pull request as ready for review October 26, 2021 16:26
Copy link
Member

@webmaster128 webmaster128 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@amaury1093 amaury1093 added the A:automerge Automatically merge PR once all prerequisites pass. label Oct 27, 2021
@mergify mergify bot merged commit 0a3660d into master Oct 27, 2021
@mergify mergify bot deleted the am/protobuf-since branch October 27, 2021 14:13
mergify bot pushed a commit that referenced this pull request Oct 27, 2021
<!--
The default pull request template is for types feat, fix, or refactor.
For other templates, add one of the following parameters to the url:
- template=docs.md
- template=other.md
-->

## Description

ref: #10406 (reply in thread)

For clients to know whether a protobuf feature is available for a certain SDK version, we decided to use the [`@since` doc comment](https://www.oracle.com/technical-resources/articles/java/javadoc-tool.html#@since) inside protobuf files.

<!-- Add a description of the changes that this PR introduces and the files that
are the most critical to review. -->

---

### Author Checklist

*All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.*

I have...

- [ ] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] added `!` to the type prefix if API or client breaking change
- [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting))
- [ ] provided a link to the relevant issue or specification
- [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules)
- [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing)
- [ ] added a changelog entry to `CHANGELOG.md`
- [ ] included comments for [documenting Go code](https://blog.golang.org/godoc)
- [ ] updated the relevant documentation or specification
- [ ] reviewed "Files changed" and left comments if necessary
- [ ] confirmed all CI checks have passed

### Reviewers Checklist

*All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.*

I have...

- [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] confirmed `!` in the type prefix if API or client breaking change
- [ ] confirmed all author checklist items have been addressed
- [ ] reviewed state machine logic
- [ ] reviewed API design and naming
- [ ] reviewed documentation is accurate
- [ ] reviewed tests and test coverage
- [ ] manually tested (if applicable)

(cherry picked from commit 0a3660d)

# Conflicts:
#	docs/core/proto-docs.md
#	proto/cosmos/bank/v1beta1/bank.proto
#	proto/cosmos/tx/v1beta1/tx.proto
#	types/tx/tx.pb.go
#	x/bank/types/bank.pb.go
amaury1093 added a commit that referenced this pull request Oct 29, 2021
* chore: Add "Since:" on proto doc comments (#10434)

<!--
The default pull request template is for types feat, fix, or refactor.
For other templates, add one of the following parameters to the url:
- template=docs.md
- template=other.md
-->

## Description

ref: https://github.com/cosmos/cosmos-sdk/discussions/10406#discussioncomment-1533289

For clients to know whether a protobuf feature is available for a certain SDK version, we decided to use the [`@since` doc comment](https://www.oracle.com/technical-resources/articles/java/javadoc-tool.html#@since) inside protobuf files.

<!-- Add a description of the changes that this PR introduces and the files that
are the most critical to review. -->

---

### Author Checklist

*All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.*

I have...

- [ ] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] added `!` to the type prefix if API or client breaking change
- [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting))
- [ ] provided a link to the relevant issue or specification
- [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules)
- [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing)
- [ ] added a changelog entry to `CHANGELOG.md`
- [ ] included comments for [documenting Go code](https://blog.golang.org/godoc)
- [ ] updated the relevant documentation or specification
- [ ] reviewed "Files changed" and left comments if necessary
- [ ] confirmed all CI checks have passed

### Reviewers Checklist

*All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.*

I have...

- [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] confirmed `!` in the type prefix if API or client breaking change
- [ ] confirmed all author checklist items have been addressed
- [ ] reviewed state machine logic
- [ ] reviewed API design and naming
- [ ] reviewed documentation is accurate
- [ ] reviewed tests and test coverage
- [ ] manually tested (if applicable)

(cherry picked from commit 0a3660d)

# Conflicts:
#	docs/core/proto-docs.md
#	proto/cosmos/bank/v1beta1/bank.proto
#	proto/cosmos/tx/v1beta1/tx.proto
#	types/tx/tx.pb.go
#	x/bank/types/bank.pb.go

* Fix conflicts

Co-authored-by: Amaury <1293565+amaurym@users.noreply.github.com>
evan-forbes pushed a commit to evan-forbes/cosmos-sdk that referenced this pull request Nov 1, 2021
…smos#10449)

* chore: Add "Since:" on proto doc comments (cosmos#10434)

<!--
The default pull request template is for types feat, fix, or refactor.
For other templates, add one of the following parameters to the url:
- template=docs.md
- template=other.md
-->

## Description

ref: cosmos#10406 (reply in thread)

For clients to know whether a protobuf feature is available for a certain SDK version, we decided to use the [`@since` doc comment](https://www.oracle.com/technical-resources/articles/java/javadoc-tool.html#@since) inside protobuf files.

<!-- Add a description of the changes that this PR introduces and the files that
are the most critical to review. -->

---

### Author Checklist

*All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.*

I have...

- [ ] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] added `!` to the type prefix if API or client breaking change
- [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting))
- [ ] provided a link to the relevant issue or specification
- [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules)
- [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing)
- [ ] added a changelog entry to `CHANGELOG.md`
- [ ] included comments for [documenting Go code](https://blog.golang.org/godoc)
- [ ] updated the relevant documentation or specification
- [ ] reviewed "Files changed" and left comments if necessary
- [ ] confirmed all CI checks have passed

### Reviewers Checklist

*All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.*

I have...

- [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] confirmed `!` in the type prefix if API or client breaking change
- [ ] confirmed all author checklist items have been addressed
- [ ] reviewed state machine logic
- [ ] reviewed API design and naming
- [ ] reviewed documentation is accurate
- [ ] reviewed tests and test coverage
- [ ] manually tested (if applicable)

(cherry picked from commit 0a3660d)

# Conflicts:
#	docs/core/proto-docs.md
#	proto/cosmos/bank/v1beta1/bank.proto
#	proto/cosmos/tx/v1beta1/tx.proto
#	types/tx/tx.pb.go
#	x/bank/types/bank.pb.go

* Fix conflicts

Co-authored-by: Amaury <1293565+amaurym@users.noreply.github.com>
Eengineer1 pushed a commit to cheqd/cosmos-sdk that referenced this pull request Aug 26, 2022
…smos#10449)

* chore: Add "Since:" on proto doc comments (cosmos#10434)

<!--
The default pull request template is for types feat, fix, or refactor.
For other templates, add one of the following parameters to the url:
- template=docs.md
- template=other.md
-->

ref: cosmos#10406 (reply in thread)

For clients to know whether a protobuf feature is available for a certain SDK version, we decided to use the [`@since` doc comment](https://www.oracle.com/technical-resources/articles/java/javadoc-tool.html#@since) inside protobuf files.

<!-- Add a description of the changes that this PR introduces and the files that
are the most critical to review. -->

---

*All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.*

I have...

- [ ] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] added `!` to the type prefix if API or client breaking change
- [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting))
- [ ] provided a link to the relevant issue or specification
- [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules)
- [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing)
- [ ] added a changelog entry to `CHANGELOG.md`
- [ ] included comments for [documenting Go code](https://blog.golang.org/godoc)
- [ ] updated the relevant documentation or specification
- [ ] reviewed "Files changed" and left comments if necessary
- [ ] confirmed all CI checks have passed

*All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.*

I have...

- [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] confirmed `!` in the type prefix if API or client breaking change
- [ ] confirmed all author checklist items have been addressed
- [ ] reviewed state machine logic
- [ ] reviewed API design and naming
- [ ] reviewed documentation is accurate
- [ ] reviewed tests and test coverage
- [ ] manually tested (if applicable)

(cherry picked from commit 0a3660d)

* Fix conflicts

Co-authored-by: Amaury <1293565+amaurym@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants