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

Linting: include sub packages #2219

Merged
merged 16 commits into from
Nov 7, 2018
Merged

Linting: include sub packages #2219

merged 16 commits into from
Nov 7, 2018

Conversation

katbyte
Copy link
Collaborator

@katbyte katbyte commented Nov 2, 2018

No description provided.

@katbyte katbyte added this to the 1.19.0 milestone Nov 2, 2018
@katbyte katbyte requested a review from a team November 2, 2018 22:08
@ghost ghost added the size/L label Nov 2, 2018
GNUmakefile Outdated Show resolved Hide resolved
@@ -163,7 +163,7 @@ func TestAzureFindValidAccessTokenForTenant_ValidFromCloudShell(t *testing.T) {
t.Fatalf("Expected the Client ID to be %q but got %q", expectedToken.ClientID, token.ClientID)
}

if token.IsCloudShell != true {
if token.IsCloudShell {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this inverted the boolean?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It sure did

Copy link
Contributor

Choose a reason for hiding this comment

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

🤔 I'd like to hold off on this PR until #2199 is merged; since otherwise there's going to be a ton of merge conflicts which'll be hard to resolve - WDYT?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

Choose a reason for hiding this comment

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

@katbyte I think this should be safe to git reset HEAD~1 && [lint] now?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@tombuildsstuff,

updated and ready to go

tokensPath, err := cli.AccessTokensPath()
if err != nil {
return fmt.Errorf("Error loading the Tokens Path from the Azure CLI: %+v", err)
tokensPath, err2 := cli.AccessTokensPath()
Copy link
Contributor

Choose a reason for hiding this comment

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

I think its fair to exclude the shadowing rule for err as its just so common, so you don't have to do this. I think using some other name for an error than err should be avoided if at all possible.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

True, however there was one or two cases i saw where the wrong error could have been printed out.

Is there an easy way to exclude shadow errors just pertaining to err?

Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

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

otherwise LGTM 👍

@@ -26,7 +26,7 @@ func ResponseErrorIsRetryable(err error) bool {
return false
}

func responseWasStatusCode(resp autorest.Response, statusCode int) bool {
func responseWasStatusCode(resp autorest.Response, statusCode int) bool { // nolint: unparam
Copy link
Contributor

Choose a reason for hiding this comment

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

which parameter is unused here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

==> Checking source code against linters...
azurerm/utils/response.go:29:52:warning: parameter statusCode always receives 404 (unparam)
make: *** [lint] Error 1

@katbyte katbyte merged commit da529be into master Nov 7, 2018
@katbyte katbyte deleted the lint/package branch November 7, 2018 23:58
@ghost
Copy link

ghost commented Mar 6, 2019

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks!

@ghost ghost locked and limited conversation to collaborators Mar 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants