-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
docs: update docs links to v0.47 (backport #14572) #14591
docs: update docs links to v0.47 (backport #14572) #14591
Conversation
(cherry picked from commit e4c0788) # Conflicts: # api/cosmos/base/snapshots/v1beta1/snapshot.pulsar.go # docs/architecture/adr-050-sign-mode-textual-annex1.md # docs/architecture/adr-050-sign-mode-textual.md # docs/docs/core/05-encoding.md # snapshots/types/snapshot.pb.go # x/circuit/README.md
@@ -66,19 +64,4 @@ message SnapshotExtensionMeta { | |||
// Since: cosmos-sdk 0.46 | |||
message SnapshotExtensionPayload { | |||
bytes payload = 1; | |||
} | |||
|
|||
// SnapshotKVItem is an exported Key/Value Pair |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AmauryM, given that is not used anywhere (as store/v2 is gone), is that okay to backport that deletion too? It is theoretically proto breaking, but because it was never used, this is okay to backport right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but because it was never used
We did release 0.46 with this proto message, right? In this case I think we need to keep it unfortunately (also on main too)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah! :( We did release it indeed, but it was only used in store/v2alpha1, which had no users and has been deleted in v0.47.
Should we deprecate it with a comment that says it is not being used anywhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's what we did for other unused fields: add a comment and the proto deprecated=true
annotation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok! I thought it only mattered for fields that were actually used. I will update then, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
id just delete it tbh. it snot used by anything and if anyone is using it then they are using an internal api externally
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is that once it's exposed in proto files, then it's really not an internal API anymore, and even JS clients can consume them.
Seems to me that there's a risk of breaking backwards-compatibility if we remove, whereas the alternative of deprecation is safe and easy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, but if anyone is using this then they are doing something wrong so I still think deleting is simpler than legacy/dead code sitting around
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Merging this, and let's reach consensus here: #14597 (as it needs to be re-added on main anyway, if we keep it)
After merge, we need to trigger the docs deployment action from main (https://github.com/cosmos/cosmos-sdk/actions/workflows/deploy-docs.yml) so it takes updates |
7373c11
to
ca01b09
Compare
This is an automatic backport of pull request #14572 done by Mergify.
Cherry-pick of e4c0788 has failed:
To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/github/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally
Mergify commands and options
More conditions and actions can be found in the documentation.
You can also trigger Mergify actions by commenting on this pull request:
@Mergifyio refresh
will re-evaluate the rules@Mergifyio rebase
will rebase this PR on its base branch@Mergifyio update
will merge the base branch into this PR@Mergifyio backport <destination>
will backport this PR on<destination>
branchAdditionally, on Mergify dashboard you can:
Finally, you can contact us on https://mergify.com