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

Fix the system-tag cli help default values #1219

Merged
merged 4 commits into from
Oct 29, 2019

Conversation

mstoykov
Copy link
Contributor

@mstoykov mstoykov commented Oct 28, 2019

this was broken in #1148

@mstoykov mstoykov added this to the v0.26.0 milestone Oct 28, 2019
@mstoykov mstoykov requested review from imiric, na-- and cuonglm October 28, 2019 14:42
@mstoykov
Copy link
Contributor Author

There is already a String() string method from the enumer so I decided to leave that one alone and add another implementation just for this case.
The naming was problematic, I first called it HelpString, but later decided that it's stupid.

@@ -70,6 +71,17 @@ func (i *SystemTagSet) Map() TagSet {
return m
}

// SetString is returns comma separeted list of the string representation of all values in the set
Copy link
Contributor

@cuonglm cuonglm Oct 28, 2019

Choose a reason for hiding this comment

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

Typo in "is returns".

And also, should this method named List(), for alignment with Map() above? And actually return a slice instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed the typo and did some .. performance optimizations (not actually needed I am sure).

I don't think it's worth it to add a generic way to get a list of the set values from the SystemTags ... this is literally the only place where it's used ...

na--
na-- previously approved these changes Oct 28, 2019
@codecov-io
Copy link

codecov-io commented Oct 28, 2019

Codecov Report

Merging #1219 into master will increase coverage by <.01%.
The diff coverage is 88.88%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1219      +/-   ##
==========================================
+ Coverage   73.75%   73.75%   +<.01%     
==========================================
  Files         147      147              
  Lines       10675    10681       +6     
==========================================
+ Hits         7873     7878       +5     
- Misses       2343     2344       +1     
  Partials      459      459
Impacted Files Coverage Δ
cmd/options.go 67.76% <100%> (ø) ⬆️
stats/system_tag.go 81.13% <85.71%> (+2.4%) ⬆️
stats/system_tag_set_gen.go 61.53% <0%> (-7.7%) ⬇️

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 c2d73ba...3dae038. Read the comment docs.

cuonglm
cuonglm previously approved these changes Oct 28, 2019
@@ -70,6 +70,17 @@ func (i *SystemTagSet) Map() TagSet {
return m
}

// SetString returns comma separeted list of the string representation of all values in the set
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo: "separeted"

imiric
imiric previously approved these changes Oct 28, 2019
Copy link
Contributor

@imiric imiric left a comment

Choose a reason for hiding this comment

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

LGTM besides the small typo.

@mstoykov mstoykov dismissed stale reviews from imiric, cuonglm, and na-- via 3dae038 October 29, 2019 05:41
@mstoykov mstoykov merged commit 9e1c5c1 into master Oct 29, 2019
@mstoykov mstoykov deleted the fixSystemTagsCLIDefaultOptionsHelp branch October 29, 2019 07:38
@mstoykov mstoykov changed the title Fix the system-tag cli help default values after #1148 Fix the system-tag cli help default values Oct 29, 2019
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.

5 participants