Repository: apigee/registry
Pull request #930 (2023-01-11 21:04:08)

Fix some previously-undetected style concerns in `registry check` flags.
I noticed that our new registry tool style checker was missing some flags because it was iterating over `Flags()` and not `LocalFlags()` *shrug* and when I changed that, I found that the missed flags were declared with `PersistentFlag()` instead of `Flag()` - @theganyo, was that needed? I changed it to `Flag()` for consistency but could change it back if you prefer. I also changed the checker to hopefully catch more problems and saw that problems in the check flags were still seen when I iterate over `LocalFlags()` (both `PersistentFlag()` and `Flag()`-based).

codecov[bot]: # [Codecov](https://codecov.io/gh/apigee/registry/pull/930?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apigee) Report
> Merging [#930](https://codecov.io/gh/apigee/registry/pull/930?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apigee) (df62954) into [main](https://codecov.io/gh/apigee/registry/commit/87fe83daec822fe81dcc0c930e0c5dc70556e2c5?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apigee) (87fe83d) will **not change** coverage.
> The diff coverage is `100.00%`.

> :exclamation: Current head df62954 differs from pull request most recent head 14f8dbb. Consider uploading reports for the commit 14f8dbb to get more accurate results

```diff
@@           Coverage Diff           @@
##             main     #930   +/-   ##
=======================================
  Coverage   63.77%   63.77%           
=======================================
  Files         111      111           
  Lines       10035    10035           
=======================================
  Hits         6400     6400           
  Misses       3015     3015           
  Partials      620      620           
```


| [Impacted Files](https://codecov.io/gh/apigee/registry/pull/930?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apigee) | Coverage Δ | |
|---|---|---|
| [cmd/registry/cmd/check/check.go](https://codecov.io/gh/apigee/registry/pull/930?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apigee#diff-Y21kL3JlZ2lzdHJ5L2NtZC9jaGVjay9jaGVjay5nbw==) | `48.07% <100.00%> (ø)` | |

:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apigee)

timburks: @theganyo [This](https://github.com/spf13/cobra/blob/4fa4fdf5cd34cb78973561d077461caea8eeb48a/command.go#L1619) is what I'm trusting to prefer `LocalFlags()` over `Flags()`, which is defined above and lacks this call.
theganyo: Yet Flags() says:
// Flags returns the complete FlagSet that applies
// to this command (local and persistent declared here and by all parents).


theganyo: Hmm. Looking around that code, I believe that Flags() _should_ actually also make the mergePersistentFlags() call. Also, LocalFlags() has a filter against persistent flags... so I'm unsure what you're actually seeing here? Hmm.

Patch:
diff --git a/cmd/registry/cmd/check/check.go b/cmd/registry/cmd/check/check.go
index d029b10..f94f99e 100644
--- a/cmd/registry/cmd/check/check.go
+++ b/cmd/registry/cmd/check/check.go
@@ -93,8 +93,8 @@ func Command() *cobra.Command {
 		},
 	}
 
-	cmd.PersistentFlags().String("filter", "", "Filter selected resources")
-	cmd.PersistentFlags().Int("jobs", 10, "Number of actions to perform concurrently")
+	cmd.Flags().String("filter", "", "filter selected resources")
+	cmd.Flags().IntP("jobs", "j", 10, "number of actions to perform concurrently")
 
 	return cmd
 }


Test patch:
diff --git a/cmd/registry/cmd/root_test.go b/cmd/registry/cmd/root_test.go
index ed853a7..8c008f4 100644
--- a/cmd/registry/cmd/root_test.go
+++ b/cmd/registry/cmd/root_test.go
@@ -67,7 +67,7 @@ func checkCommand(t *testing.T, cmd *cobra.Command, prefix string) {
 				}
 			}
 			// Check field usage messages.
-			flags := cmd.Flags()
+			flags := cmd.LocalFlags()
 			flags.VisitAll(func(f *pflag.Flag) {
 				if f.Usage == "" {
 					t.Errorf("%q %q flag usage must not be empty.", name, f.Name)