-
Notifications
You must be signed in to change notification settings - Fork 86
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: move flags to option
package
#1195
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Junjie Gao <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1195 +/- ##
=======================================
Coverage 76.22% 76.22%
=======================================
Files 71 70 -1
Lines 3634 3634
=======================================
Hits 2770 2770
Misses 668 668
Partials 196 196 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Junjie Gao <[email protected]>
Signed-off-by: Junjie Gao <[email protected]>
option
package
option
packageoption
package
option
packageoption
package
Signed-off-by: Junjie Gao <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -12,7 +12,7 @@ | |||
// limitations under the License. | |||
|
|||
// Package cmd contains common flags and routines for all CLIs. | |||
package cmd | |||
package option |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well... After reading through the PR, I personally prefer the previous structure, where internal/cmd/flags.go
is clear enough for any developer to add a new flag in it.
From description at line 14: // Package cmd contains common flags and routines for all CLIs.
This description on package option
leaves a deep concern, and my question is: Why do we have to keep creating new PRs to just move around internal files, renaming the package names with something else, while reducing readability of the code base and increasing developer mistakes like above? Is this really a MUST to do item?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are two issues in the repo:
- Flags and options are located in multiple places: the
internal/cmd
package andcmd/notation/common.go
. - The
internal/cmd
package name is too broad. What doescmd
mean? Everything we are working on is command-related.
The option
package only includes flags and options that are used when defining a command opts struct, so I think this name is a better choice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I mentioned in the above comment, your description to package option
is already wrong, I'm pretty sure this is a reduction of readability and increases developing difficulty for others.
If you really want this chore PR, my suggestion is:
Move flag related code from common.go
to internal/cmd/flags.go
and that's it.
Moved
internal/cmd/flags.go
,internal/cmd/options.go
andcmd/notation/common.go
tooption
package to concentrate all the flags and options related code in one place.