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: boolean logic #98

Merged
merged 2 commits into from
Sep 23, 2020
Merged

fix: boolean logic #98

merged 2 commits into from
Sep 23, 2020

Conversation

busser
Copy link
Contributor

@busser busser commented Sep 22, 2020

Fix #93

@codecov-commenter
Copy link

codecov-commenter commented Sep 22, 2020

Codecov Report

Merging #98 into master will decrease coverage by 16.62%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master      #98       +/-   ##
===========================================
- Coverage   52.40%   35.77%   -16.63%     
===========================================
  Files           8        9        +1     
  Lines         250      341       +91     
===========================================
- Hits          131      122        -9     
- Misses         97      197      +100     
  Partials       22       22               
Impacted Files Coverage Δ
internal/client/client.go 10.56% <100.00%> (ø)
cmd/version.go 60.00% <0.00%> (-6.67%) ⬇️
internal/printer/adapter.go 73.33% <0.00%> (-6.67%) ⬇️
internal/version/version.go 22.22% <0.00%> (-5.06%) ⬇️
cmd/completion.go 52.00% <0.00%> (-3.56%) ⬇️
cmd/root.go 66.66% <0.00%> (-2.90%) ⬇️
internal/options/options.go 16.66% <0.00%> (-2.39%) ⬇️
internal/filter/filter.go 73.01% <0.00%> (-1.64%) ⬇️
internal/printer/table_printer.go 0.00% <0.00%> (ø)

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 071f5c7...32650f0. Read the comment docs.

Copy link
Owner

@corneliusweig corneliusweig left a comment

Choose a reason for hiding this comment

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

🤦‍♂️ This clearly shows why tests pay off!
Thanks for stepping up and fixing this!

Before I can merge, I need to ask you though to signoff your commit (see here).

Comment on lines +7 to +10
scope string
wantCluster bool
wantNamespace bool
wantErr bool
Copy link
Owner

Choose a reason for hiding this comment

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

No need for change, just a thought: If you introduce a struct like this

type result struct {
  cluster, namespace, err bool
}

you can do all three comparisons at once like so:

if diff := cmp.Diff(test.want, got); diff != "" {
  t.Errorf("getResourceScope(..) returned unexpected result (-want,+got)\n%s", diff)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Neat! I'll keep that in mind for next time

Signed-off-by: Arthur Busser <arthur.busser@gmail.com>
Signed-off-by: Arthur Busser <arthur.busser@gmail.com>
@busser
Copy link
Contributor Author

busser commented Sep 23, 2020

@corneliusweig Commits are signed off 👍

@corneliusweig corneliusweig merged commit 751a3d1 into corneliusweig:master Sep 23, 2020
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.

No resources returned in v1.3.4
3 participants