General refactor of the cmd package #3328
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3328 +/- ##
==========================================
+ Coverage 35.05% 35.06% +<.01%
==========================================
Files 280 281 +1
Lines 40565 40559 -6
==========================================
Hits 14221 14221
+ Misses 24241 24235 -6
Partials 2103 2103
Continue to review full report at Codecov.
|
tboerger
added
the
lgtm/need 2
| } | ||
|
|
||
| var priv interface{} | ||
| var err error | ||
| switch ctx.String("ecdsa-curve") { | ||
| switch c.String("ecdsa-curve") { |
There was a problem hiding this comment.
rename of ctx to c just makes the code less readable, please change back
There was a problem hiding this comment.
I would generally agree, however the entire package refers to c *cli.Context instead of ctx *cli.Context. Shall I change everything to use ctx?
tboerger
added
lgtm/need 1
cmd/admin.go
Outdated
| return fmt.Errorf("Password is not specified") | ||
| } else if !c.IsSet("email") { | ||
| return fmt.Errorf("Email is not specified") | ||
| if err := argsSet(c, "username", "password"); err != nil { |
There was a problem hiding this comment.
This is breaking change and also (name -> username) and also is missing email check
There was a problem hiding this comment.
Wooops, looks like I forgot to edit after copypasting - nice catch! (good thing there are code reviews 😌)
lafriks
added
the
type/refactoring
tboerger
added
lgtm/done
cmd/admin.go
Outdated
|
|
||
| // argsSet checks that all the required arguments are set. args is a list of | ||
| // arguments that must be set in the passed Context. | ||
| func argsSet(c *cli.Context, args ...string) error { |
There was a problem hiding this comment.
I'd move these two functions into cmd/common.go or similar
|
@bkcsoft needs your approval |
Mostly some refactoring and removing of spaghetti code - nothing major.