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: cli: cleanup cli #10114

Merged
merged 3 commits into from
Jan 26, 2023
Merged

chore: cli: cleanup cli #10114

merged 3 commits into from
Jan 26, 2023

Conversation

rjan90
Copy link
Contributor

@rjan90 rjan90 commented Jan 25, 2023

Related Issues

Standarize cli/code functions similar to: #9317

Proposed Changes

  • Change to cctx.NArg() instead of cctx.Args().xxx
  • Add ArgsUsage to commands that was missing it
  • Add check for args and print help on functions that did not have it

Checklist

Before you mark the PR ready for review, please make sure that:

  • Commits have a clear commit message.
  • PR title is in the form of of <PR type>: <area>: <change being made>
    • example: fix: mempool: Introduce a cache for valid signatures
    • PR type: fix, feat, build, chore, ci, docs, perf, refactor, revert, style, test
    • area, e.g. api, chain, state, market, mempool, multisig, networking, paych, proving, sealing, wallet, deps
  • New features have usage guidelines and / or documentation updates in
  • Tests exist for new functionality or change in behavior
  • CI is green

Standarize cli/code functions similar to: #9317

- cctx.NArg() instead of cctx.Args().xxx
- Add check for args and print help on functions that did not have it
@rjan90 rjan90 requested a review from a team as a code owner January 25, 2023 12:20
@geoff-vball
Copy link
Contributor

Thanks, this looks great! Just needs a make docsgen-cli

make docsgen-cli
@geoff-vball
Copy link
Contributor

@rjan90 I've rerun the flaky tests a few times. It looks like the remaining failures are real. Looks like strictly checking the number of params in a few cases caused problems. Could be the test that's wrong

@rjan90
Copy link
Contributor Author

rjan90 commented Jan 25, 2023

@geoff-vball Yeah, looks like there is issues with the lotus wallet markets add test, I will check it up tomorrow

Less strict ArgsCheck
@rjan90
Copy link
Contributor Author

rjan90 commented Jan 26, 2023

Looks like all checks have passed now. I ended up having a not so strict ArgCheck for the lotus wallet markets add cmd. The failure seemed to come from a discrepency in the CLI and test for MarketAddBalance.

While MarketAddBalance needs wallet, addr and amt, the cli-function only asks for amt and defaults to the default address for wallet and addr if not anything else is specified through the options --address and --from. So there is potentially an edge case failure in the cli, where it will fail if lotus has not an default adress set and the wallet and addr is not set through the options.

@geoff-vball
Copy link
Contributor

Awesome, that's totally fine by me.

@geoff-vball geoff-vball merged commit c45df78 into master Jan 26, 2023
@geoff-vball geoff-vball deleted the fix/cli-cleanup branch January 26, 2023 14:25
@rjan90 rjan90 mentioned this pull request May 3, 2023
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants