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

fix: remove support for NoOptDefaultValue #14985

Merged
merged 3 commits into from
Feb 9, 2023
Merged

fix: remove support for NoOptDefaultValue #14985

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3251029
fix: remove support for NoOptDefaultValue
JeancarloBarrios Feb 9, 2023
708b011
Merge branch 'main' into JeancarloBarrios/remove-no-opt-from-autocli
JeancarloBarrios Feb 9, 2023
40d24f7
Merge branch 'main' into JeancarloBarrios/remove-no-opt-from-autocli
JeancarloBarrios Feb 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
130 changes: 27 additions & 103 deletions api/cosmos/autocli/v1/options.pulsar.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 0 additions & 1 deletion client/v2/autocli/flag/register.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,6 @@ func (b *Builder) addMessageFlags(ctx context.Context, flagSet *pflag.FlagSet, m
flag.Deprecated = opts.Deprecated
flag.ShorthandDeprecated = opts.ShorthandDeprecated
flag.Hidden = opts.Hidden
flag.NoOptDefVal = opts.NoOptDefaultValue
}
})

Expand Down
4 changes: 2 additions & 2 deletions client/v2/autocli/query_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,8 +53,8 @@ var testCmdDesc = &autocliv1.ServiceCommandDescriptor{
DefaultValue: "3",
},
"u64": {
Usage: "some random uint64",
NoOptDefaultValue: "5",
Usage: "some random uint64",
DefaultValue: "5",
},
"deprecated_field": {
Deprecated: "don't use this",
Expand Down
3 changes: 0 additions & 3 deletions proto/cosmos/autocli/v1/options.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,9 +101,6 @@ message FlagOptions {
// default_value is the default value as text.
string default_value = 4;

// default value is the default value as text if the flag is used without any value.
string no_opt_default_value = 5;

// deprecated is the usage text to show if this flag is deprecated.
string deprecated = 6;
Copy link
Member

@julienrbrt julienrbrt Feb 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Actually, as we are deleting a field, maybe we should decrease the identifier of the others too, given it has not been yet released. cc @JeancarloBarrios

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True I missed that thanks !!

Copy link
Member

Choose a reason for hiding this comment

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

I would suggest we don't do this actually... so the issue is that this is part of the api module not 0.47 at all. So backporting doesn't really do anything. The thing that we need to do is tag api and then backport the required API module version to 0.47. Removing the field is one thing, but changing the numbers really would break any case that someone built their binary with an earlier version of api. This is maybe the first case in the wild where the issues raised in ADR 046 can be observed. Let's discuss a tagging/versioning strategy next week.

Copy link
Member

@julienrbrt julienrbrt Feb 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This makes sense! Didn't notice the generated proto was pulsar.
One small unrelated issue is that now API possibly not support v0.47 entirely given CometBFT (#14899)


Expand Down