Improve linting and fix found issues #435

ferhatelmas · 2019-12-13T01:31:29Z

Enable all rules from gocritic except some troublesome rules (see .golangci.yml for details).

Here is a part of what new rules bring:

don't shadow imports or builtins such as
os and close respectively.
use wrapper ReplaceAll
combine types in func calls if same type
find duplicate imports
find suspicious regex
ensure proper comment format

To fix some of the issues, I created a type for os and arch, then exported it.

* don't shadow imports or builtins such as `os` and `close` respectively. * use wrapper ReplaceAll * combine types in func calls if same type

codecov-io · 2019-12-13T01:39:33Z

Codecov Report

Merging #435 into master will decrease coverage by 0.1%.
The diff coverage is 78.18%.

@@            Coverage Diff             @@
##           master     #435      +/-   ##
==========================================
- Coverage   56.35%   56.25%   -0.11%     
==========================================
  Files          19       19              
  Lines         928      928              
==========================================
- Hits          523      522       -1     
- Misses        350      351       +1     
  Partials       55       55

Impacted Files	Coverage Δ
internal/installation/upgrade.go	`0% <0%> (ø)`	⬆️
internal/download/verifier.go	`100% <100%> (ø)`	⬆️
internal/receiptsmigration/migration.go	`46.15% <100%> (ø)`	⬆️
internal/installation/install.go	`42.42% <100%> (ø)`	⬆️
internal/download/downloader.go	`68.14% <60%> (-0.28%)`	⬇️
internal/installation/platform.go	`76.92% <80%> (-2.25%)`	⬇️
cmd/validate-krew-manifest/main.go	`54.08% <81.48%> (-0.47%)`	⬇️

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 541b48b...4a908b4. Read the comment docs.

ahmetb · 2019-12-13T01:43:23Z

thanks for the work. looks good.
is there a linter option to catch these in the first place?

ferhatelmas · 2019-12-13T07:08:08Z

@ahmetb It was mostly from my reading but checked linters and added more rules
and it found more issues so I added more refactoring.

ferhatelmas · 2019-12-13T07:13:20Z

cmd/validate-krew-manifest/main.go

 	})
 }

 // allPlatforms returns all <os,arch> pairs krew is supported on.
-func allPlatforms() [][2]string {
+func allPlatforms() []installation.GoOSArch {
 	// TODO(ahmetb) find a more authoritative source for this list


go tool dist list

well, now we now how to get a complete list :)

So what shall we do about it?

I am aware of that. But that would require us running a cmd and keeping code up to date periodically. This is ok for now.

corneliusweig · 2019-12-13T13:18:11Z

/lgtm

ahmetb · 2019-12-13T18:39:31Z

internal/installation/platform_test.go

-		t.Errorf("returned OS=%q; expected=%q", outOS, inOS)
+	out := OSArch()
+	if inOS != out.OS {
+		t.Errorf("returned OS=%q; expected=%q", out.OS, inOS)


Use cmp.diff here
What if we add another field :)

ahmetb · 2019-12-13T18:40:08Z

internal/installation/platform.go

@@ -55,16 +55,29 @@ func matchPlatform(platforms []index.Platform, os, arch string) (index.Platform,
 	return index.Platform{}, false, nil
 }

-// osArch returns the OS/arch combination to be used on the current system. It
+// GoOSArch is wrapper around operating system and architecture
+type GoOSArch struct {


Can we call this Platform or is that too generic?
We don’t care about “Go” part that much.

It can be used in index too (since exported I didn't want to change in a lint PR).
They are also go specific so for now, it might leave as it is.

🤔 Wasn't GoOSArch just added in this PR? I think we can choose whatever name suits best. Besides, everything is internal now, so we don't have to bother.

Yes, I rename to whatever you want.

What I mean is that with a better name it could be used in non-internal places too.

Ok :)

So what about Platform as proposed by Ahmet?

Sounds good to me since it's in the platform package. Renaming unless there is a better suggestion.

ClientPlatform or OSArchPair isn't that bad either.

I think renaming it to Platform could be misleading with index.Platform.

It's just OS and Arch so a simple naming might be better. OSArchPair sounds somehow better to me.

ahmetb · 2019-12-15T03:05:49Z

checked linters and added more rules

Is there a way we can add these linters preventing shadowing builtins to the CI validation, so that we don't have to go through this again?

ferhatelmas · 2019-12-15T14:00:33Z

Is there a way we can add these linters preventing shadowing builtins to the CI validation

I already added, see the diff in .golangci.yml.

ferhatelmas · 2019-12-18T21:59:37Z

Anything else is needed?

ahmetb · 2019-12-18T22:09:36Z

Thanks. I don't fully understand the list of linter check in .golangci.yml, but it's ok for now.
/lgtm
/approve

k8s-ci-robot · 2019-12-18T22:10:06Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahmetb, ferhatelmas

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [ahmetb]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 13, 2019

k8s-ci-robot requested review from corneliusweig and soltysh December 13, 2019 01:31

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 13, 2019

Some refactoring improvements

1273c64

* don't shadow imports or builtins such as `os` and `close` respectively. * use wrapper ReplaceAll * combine types in func calls if same type

ferhatelmas force-pushed the small-improvements branch from d57022a to 1273c64 Compare December 13, 2019 01:33

More refactoring and golangci config improvement

7a79337

Run go mod tidy

0bf3d63

ferhatelmas commented Dec 13, 2019

View reviewed changes

ferhatelmas changed the title ~~Some refactoring improvements~~ Improve linting and fix found issues Dec 13, 2019

k8s-ci-robot assigned corneliusweig Dec 13, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 13, 2019

ahmetb reviewed Dec 13, 2019

View reviewed changes

Use cmp in tests

3bbfda8

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 14, 2019

Rename GoOSArch to OSArchPair

4a908b4

k8s-ci-robot assigned ahmetb Dec 18, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 18, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 18, 2019

k8s-ci-robot merged commit d7bc0ca into kubernetes-sigs:master Dec 18, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve linting and fix found issues #435

Improve linting and fix found issues #435

ferhatelmas commented Dec 13, 2019 •

edited

Loading

codecov-io commented Dec 13, 2019 •

edited

Loading

ahmetb commented Dec 13, 2019 •

edited

Loading

ferhatelmas commented Dec 13, 2019

ferhatelmas Dec 13, 2019

corneliusweig Dec 13, 2019

ahmetb Dec 13, 2019

corneliusweig commented Dec 13, 2019

ahmetb Dec 13, 2019

ferhatelmas Dec 14, 2019

ahmetb Dec 13, 2019

ferhatelmas Dec 14, 2019

corneliusweig Dec 14, 2019 •

edited

Loading

ferhatelmas Dec 14, 2019

corneliusweig Dec 14, 2019

ferhatelmas Dec 14, 2019

ahmetb Dec 15, 2019

ferhatelmas Dec 15, 2019

ahmetb commented Dec 15, 2019

ferhatelmas commented Dec 15, 2019

ferhatelmas commented Dec 18, 2019

ahmetb commented Dec 18, 2019

k8s-ci-robot commented Dec 18, 2019

Improve linting and fix found issues #435

Improve linting and fix found issues #435

Conversation

ferhatelmas commented Dec 13, 2019 • edited Loading

codecov-io commented Dec 13, 2019 • edited Loading

Codecov Report

ahmetb commented Dec 13, 2019 • edited Loading

ferhatelmas commented Dec 13, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

corneliusweig commented Dec 13, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

corneliusweig Dec 14, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahmetb commented Dec 15, 2019

ferhatelmas commented Dec 15, 2019

ferhatelmas commented Dec 18, 2019

ahmetb commented Dec 18, 2019

k8s-ci-robot commented Dec 18, 2019

ferhatelmas commented Dec 13, 2019 •

edited

Loading

codecov-io commented Dec 13, 2019 •

edited

Loading

ahmetb commented Dec 13, 2019 •

edited

Loading

corneliusweig Dec 14, 2019 •

edited

Loading