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

refactor: Improve CLI error handling and fix small issues #649

Merged
merged 7 commits into from
Jul 23, 2022

Conversation

orpheuslummis
Copy link
Contributor

@orpheuslummis orpheuslummis commented Jul 21, 2022

Relevant issue(s)

Resolves #271
Resolves #613

Description

  • adding license and copyright info to root command
  • general tidy
  • fix the command server-dump creating a new badger directory by itself
  • converted all commands to use "bubbling up" style error handling
  • all flags documentation to start with capital letters
  • revamp command texts
  • "controlled display of commands' Usage & erroring"
  • update in-repo command docs
  • usage of command tree's Context instead of command-local context

Tasks

  • I made sure the code is well commented, particularly hard-to-understand areas.
  • I made sure the repository-held documentation is changed accordingly.
  • I made sure the pull request title adheres to the conventional commit style (the subset used in the project can be found in tools/configs/chglog/config.yml).
  • I made sure to discuss its limitations such as threats to validity, vulnerability to mistake and misuse, robustness to invalidation of assumptions, resource requirements, ...

How has this been tested?

Manual...

Specify the platform(s) on which this was tested:

  • Debian
  • MacOS

@orpheuslummis orpheuslummis added the area/cli Related to the CLI binary label Jul 21, 2022
@orpheuslummis orpheuslummis added this to the DefraDB v0.3 milestone Jul 21, 2022
@orpheuslummis orpheuslummis self-assigned this Jul 21, 2022
@orpheuslummis orpheuslummis added the action/no-benchmark Skips the action that runs the benchmark. label Jul 21, 2022
@sourcenetwork sourcenetwork deleted a comment from source-devs Jul 21, 2022
@sourcenetwork sourcenetwork deleted a comment from source-devs Jul 21, 2022
@orpheuslummis orpheuslummis marked this pull request as ready for review July 21, 2022 23:38
cli/init.go Outdated Show resolved Hide resolved
cli/rpc.go Outdated Show resolved Hide resolved
cli/schema.go Outdated Show resolved Hide resolved
cli/serverdump.go Outdated Show resolved Hide resolved
cli/start.go Outdated Show resolved Hide resolved
cli/start.go Outdated Show resolved Hide resolved
cli/start.go Outdated Show resolved Hide resolved
cli/addreplicator.go Show resolved Hide resolved
@fredcarle
Copy link
Collaborator

In general it looks really good Orphée. Only a few minor suggestion for now. I'll look more closely at the docs tomorrow :)

Copy link
Member

@jsimnz jsimnz left a comment

Choose a reason for hiding this comment

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

praise: Appreciate the cleaner error handling, and really appreciate the attention to the context objects, and being consistent with their usage via cmd.Context instead of re-creating on each case.

Had a question regarding automatic vs manual arg length checking if len(args) vs Args: cobra.ExactArgs(...)

I know that Fred also brought this point up, and I guess there is a divergence of preference.

To be clear, im all for the manual checking, assuming that the automatic checking via ExactArgs isn't actually producing the usage string when it fails validation (incorrect number)

If the automatic checking does produce the usage string, i'd prefer to use that.

cli/blocks_get.go Show resolved Hide resolved
cli/addreplicator.go Show resolved Hide resolved
@orpheuslummis
Copy link
Contributor Author

@jsimnz it's more control allowing to display usage help and specific error message. The alternative doesn't display a nice message.

@jsimnz
Copy link
Member

jsimnz commented Jul 22, 2022

@jsimnz it's more control allowing to display usage help and specific error message. The alternative doesn't display a nice message.

Gotcha, I figured the alternative would, in which case, I agree to these changes 👍

Copy link
Collaborator

@fredcarle fredcarle left a comment

Choose a reason for hiding this comment

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

👏

@orpheuslummis orpheuslummis force-pushed the orpheus/refactor/cli-improvements branch from 85a7ca9 to cbca4ef Compare July 23, 2022 10:37
@codecov
Copy link

codecov bot commented Jul 23, 2022

Codecov Report

Merging #649 (85a7ca9) into develop (355ee34) will decrease coverage by 0.21%.
The diff coverage is n/a.

❗ Current head 85a7ca9 differs from pull request most recent head cbca4ef. Consider uploading reports for the commit cbca4ef to get more accurate results

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #649      +/-   ##
===========================================
- Coverage    57.32%   57.11%   -0.22%     
===========================================
  Files          122      122              
  Lines        14753    14652     -101     
===========================================
- Hits          8457     8368      -89     
+ Misses        5573     5567       -6     
+ Partials       723      717       -6     
Impacted Files Coverage Δ
query/graphql/planner/group.go 81.05% <0.00%> (-2.47%) ⬇️
query/graphql/schema/generate.go 84.41% <0.00%> (-0.18%) ⬇️
query/graphql/mapper/mapper.go 87.79% <0.00%> (-0.08%) ⬇️
query/graphql/planner/explain.go 68.96% <0.00%> (ø)
query/graphql/schema/types/types.go 100.00% <0.00%> (ø)
query/graphql/planner/arbitrary_join.go 79.55% <0.00%> (ø)

@orpheuslummis orpheuslummis merged commit 8dce502 into develop Jul 23, 2022
@orpheuslummis orpheuslummis deleted the orpheus/refactor/cli-improvements branch July 23, 2022 10:42
shahzadlone pushed a commit to shahzadlone/defradb that referenced this pull request Feb 23, 2024
…ork#649)

Resolves sourcenetwork#271 
Resolves sourcenetwork#613 

- adding license and copyright info to root command
- general tidy
- fix the command server-dump creating a new badger directory by itself
- converted all commands to use "bubbling up" style error handling
- all flags documentation to start with capital letters
- revamp command texts
- "controlled display of commands' Usage & erroring"
- update in-repo command docs
- usage of command tree's Context instead of command-local context
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
action/no-benchmark Skips the action that runs the benchmark. area/cli Related to the CLI binary
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CLI: controlled display of commands' Usage (and errors) [Epic] CLI overhaul v0.3
4 participants