Feat/silence print #393

hbagdi · 2021-05-18T23:19:04Z

hbagdi · 2021-05-18T23:19:59Z

Please review commit by commit.
I'm looking for feedback on:

flag naming
how to lay out code (continued effort to de-duplicate code)

codecov-commenter · 2021-05-18T23:22:21Z

Codecov Report

Merging #393 (39571ab) into main (3423945) will increase coverage by 0.01%.
The diff coverage is 26.08%.

@@            Coverage Diff             @@
##             main     #393      +/-   ##
==========================================
+ Coverage   47.02%   47.03%   +0.01%     
==========================================
  Files          87       88       +1     
  Lines        6025     6036      +11     
==========================================
+ Hits         2833     2839       +6     
- Misses       2869     2874       +5     
  Partials      323      323

Impacted Files	Coverage Δ
cmd/common.go	`0.00% <0.00%> (ø)`
cmd/common_konnect.go	`15.21% <0.00%> (+0.63%)`	⬆️
cmd/utils.go	`22.22% <22.22%> (ø)`
cmd/konnect_diff.go	`60.86% <25.00%> (-7.56%)`	⬇️
cmd/konnect_sync.go	`52.63% <25.00%> (-7.37%)`	⬇️
cmd/diff.go	`73.52% <50.00%> (+0.80%)`	⬆️
cmd/sync.go	`73.52% <50.00%> (+0.80%)`	⬆️

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 3423945...39571ab. Read the comment docs.

rainest · 2021-05-19T16:58:40Z

cmd/utils.go

+	set.BoolVar(&silenceEvents, "disable printing events to stdout",
+		false, "silent events")


Is the arg order here correct? It looks like you have two usage strings and no name: https://pkg.go.dev/github.com/spf13/pflag#FlagSet.BoolVar

silence-events seems fine if this is only a partial silence. If it's actually everything, then silence.

Thanks for catching that, the last refactor gone wrong. Updated.

This patch also lays out code in a way to avoid duplication across commands. decK's commands share significant amount of flags and a code pattern like this can minimize the amount of code and number of code paths that exist with flag parsing.

hbagdi requested a review from a team as a code owner May 18, 2021 23:19

hbagdi linked an issue May 18, 2021 that may be closed by this pull request

[Feature] - Allow the ability to mute or not display plugin config when syncing #232

Closed

shaneutt approved these changes May 19, 2021

View reviewed changes

rainest reviewed May 19, 2021

View reviewed changes

hbagdi force-pushed the feat/silence-print branch from 284ac11 to 06f1bd1 Compare May 19, 2021 17:02

hbagdi requested a review from mflendrich May 21, 2021 21:29

hbagdi added 2 commits May 21, 2021 14:30

refactor: DRY out print of summary

83efde7

hbagdi force-pushed the feat/silence-print branch from 06f1bd1 to 39571ab Compare May 21, 2021 21:30

hbagdi merged commit 58789ef into main May 24, 2021

hbagdi deleted the feat/silence-print branch May 24, 2021 21:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat/silence print #393

Feat/silence print #393

hbagdi commented May 18, 2021

hbagdi commented May 18, 2021

codecov-commenter commented May 18, 2021 •

edited

Loading

rainest May 19, 2021

hbagdi May 19, 2021

		set.BoolVar(&silenceEvents, "disable printing events to stdout",
		false, "silent events")

Feat/silence print #393

Feat/silence print #393

Conversation

hbagdi commented May 18, 2021

hbagdi commented May 18, 2021

codecov-commenter commented May 18, 2021 • edited Loading

Codecov Report

rainest May 19, 2021

Choose a reason for hiding this comment

hbagdi May 19, 2021

Choose a reason for hiding this comment

codecov-commenter commented May 18, 2021 •

edited

Loading