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

Add CLI Feature: Hot commands #406

Merged
merged 8 commits into from
Nov 26, 2021
Merged

Conversation

sakshamb2113
Copy link
Contributor

@sakshamb2113 sakshamb2113 commented Oct 30, 2021

This PR is for onflow/flip-fest#14

Description

  • Customised the help screen to display commands classified into 2 types.
  • Added 2 new "Hot Commands"- deploy and run.
  • Used Annotation field in the cobra.Command class to classify the commands.
  • Now help screen looks as shown below
    Screenshot from 2021-10-30 17-43-00

For contributor use:

  • Targeted PR against master branch
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work
  • Code follows the standards mentioned here
  • Updated relevant documentation
  • Re-reviewed Files changed in the Github PR explorer
  • Added appropriate labels

@sakshamb2113 sakshamb2113 changed the title Added CLI Feature: Hot commands Add CLI Feature: Hot commands Oct 30, 2021
var DeployCommand = &command.Command{
Cmd: &cobra.Command{
Use: "deploy",
Short: "Deploy contracts",
Copy link
Contributor

@sideninja sideninja Nov 2, 2021

Choose a reason for hiding this comment

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

More clarity maybe: Deploy all project contracts

"HotCommand": "true",
},
Run: func(cmd *cobra.Command, args []string) {
project.DeployCommand.Cmd.Run(cmd, args)
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't this two commands be swapped in order?

@sideninja sideninja self-requested a review November 2, 2021 08:55
Copy link
Contributor

@sideninja sideninja left a comment

Choose a reason for hiding this comment

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

Great effort so far, one thing that is problematic and needs changing is the fact that if you now use help with any of the commands you get a "hot commands" section, so for example, if I try to run flow accounts --help I will get first an empty "hot commands" section, but the way we want this to work is to only show that on the top "flow" command help.

@sakshamb2113
Copy link
Contributor Author

I have made the changes and updated the code. Could you please have a look at it now ?

Copy link
Contributor

@sideninja sideninja left a comment

Choose a reason for hiding this comment

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

Great improvement, LGTM

@codecov-commenter
Copy link

Codecov Report

Merging #406 (99b22de) into master (f27c55e) will decrease coverage by 0.08%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #406      +/-   ##
==========================================
- Coverage   55.33%   55.24%   -0.09%     
==========================================
  Files          36       36              
  Lines        1874     1877       +3     
==========================================
  Hits         1037     1037              
- Misses        707      710       +3     
  Partials      130      130              
Flag Coverage Δ
unittests 55.24% <ø> (-0.09%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pkg/flowkit/transaction.go 0.00% <0.00%> (ø)
pkg/flowkit/keys.go 32.60% <0.00%> (+0.35%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f27c55e...99b22de. Read the comment docs.

@sideninja sideninja merged commit 99b6a6d into onflow:master Nov 26, 2021
@sideninja sideninja mentioned this pull request Nov 26, 2021
6 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.

3 participants