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

fix(cli): accept flags after command's arguments #762

Merged
merged 13 commits into from
May 21, 2023

Conversation

tbruyelle
Copy link
Contributor

@tbruyelle tbruyelle commented Apr 19, 2023

Addresses #731

Description

This change allows to pass flags after a command's argument. This regression was added with the latest cli refactor that introduced the peterbourgon/ff/v3/ffcli library for command and flag parsing.

This change doesn't remove or replace ffcli, but instead suggest the minimal change to support flags after command's arguments. For that ffcli is forked inside tm2/pkg/commands/ffcli, and patched to support this feature.

The first commit just forks ffcli, while the second contains the patch, so I suggest to focus the review on this one.

Tests

Before that change, it was not possible to execute this command, because the -remote flags was expected before the auth/accounts/g1jg8mtutu9khhfwc4nxmuhcpftf0pajdhfvsqf5 query argument.

gnokey query auth/accounts/g1jg8mtutu9khhfwc4nxmuhcpftf0pajdhfvsqf5  -remote https://test3.gno.land:36657

Additional notes

I wanted to bring the minimal change, but maybe it's better to merge the forked ffcli and our legacy commands package to have a better code. Anyway this PR can be used as a preliminary work for a wider refactor.

@tbruyelle tbruyelle requested a review from a team as a code owner April 19, 2023 20:59
@moul
Copy link
Member

moul commented Apr 20, 2023

Looks nice, and the first step going in the direction of #731.

Please, fix the CI, and I'll give a round of review.

@tbruyelle
Copy link
Contributor Author

@moul I fixed the linter, but it seems I'm facing the port already in use problem. Can you rerun the test, I don't have that permission.

2023-04-20T21:18:18.3740807Z panic: failed to listen on 0.0.0.0:47768: listen tcp 0.0.0.0:47768: bind: address already in use
2023-04-20T21:18:18.3741407Z 
2023-04-20T21:18:18.3741950Z goroutine 1 [running]:
2023-04-20T21:18:18.3742491Z github.com/gnolang/gno/tm2/pkg/bft/rpc/lib.setup()
2023-04-20T21:18:18.3742891Z 	/home/runner/work/gno/gno/tm2/pkg/bft/rpc/lib/rpc_test.go:126 +0x9e7
2023-04-20T21:18:18.3743315Z github.com/gnolang/gno/tm2/pkg/bft/rpc/lib.TestMain(0xffffffffffffffff?)
2023-04-20T21:18:18.3743716Z 	/home/runner/work/gno/gno/tm2/pkg/bft/rpc/lib/rpc_test.go:86 +0x1e
2023-04-20T21:18:18.3743981Z main.main()
2023-04-20T21:18:18.3744198Z 	_testmain.go:59 +0x1d3
2023-04-20T21:18:18.3764770Z FAIL	github.com/gnolang/gno/tm2/pkg/bft/rpc/lib	0.007s

tm2/pkg/commands/commands_test.go Outdated Show resolved Hide resolved
tm2/pkg/commands/ffcli/command.go Outdated Show resolved Hide resolved
tm2/pkg/commands/ffcli/command.go Outdated Show resolved Hide resolved
@thehowl
Copy link
Member

thehowl commented May 5, 2023

diff between this and ffcli source: https://www.diffchecker.com/dtnUgZU6/

@zivkovicmilos zivkovicmilos self-requested a review May 11, 2023 18:30
@tbruyelle tbruyelle marked this pull request as draft May 11, 2023 20:18
@tbruyelle tbruyelle marked this pull request as ready for review May 12, 2023 14:56
@tbruyelle tbruyelle requested a review from jaekwon as a code owner May 12, 2023 14:56
Copy link
Member

@zivkovicmilos zivkovicmilos left a comment

Choose a reason for hiding this comment

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

I believe all gno CLI users will breathe a sigh of relief after we merge this out 🙂

Thank you for adding these changes, and for being thorough in the PR description / comments 🙏

@ajnavarro ajnavarro added 📦 🌐 tendermint v2 Issues or PRs tm2 related 🌟 improvement performance improvements, refactors ... labels May 18, 2023
This was referenced May 19, 2023
@moul
Copy link
Member

moul commented May 19, 2023

@tbruyelle can you rebase and fix conflicts? then we can merge.

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.

love this, Parse logic is clear and made me convinced it worked correctly without even seeing the tests, great job on the refactor

@tbruyelle
Copy link
Contributor Author

@tbruyelle can you rebase and fix conflicts? then we can merge.

Conflicts fixed.

@thehowl thehowl merged commit 1cc7ed9 into gnolang:master May 21, 2023
@tbruyelle tbruyelle deleted the fix/cli-flag-order branch May 22, 2023 08:58
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 🌟 improvement performance improvements, refactors ...
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants