Skip to content
This repository was archived by the owner on May 9, 2021. It is now read-only.

lint: swap IfStmt tests to not skip every other IfStmt in a chain #383

Closed
wants to merge 2 commits into from
Closed

lint: swap IfStmt tests to not skip every other IfStmt in a chain #383

wants to merge 2 commits into from

Conversation

marcelocantos
Copy link
Contributor

@marcelocantos marcelocantos commented Feb 26, 2018 •
edited
Loading

Fixes #382

The fix works by moving the test for ignored nodes after the else-node check.

@gopherbot
Copy link

This PR (HEAD: 091f8e0) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/#/c/lint/+/97256 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@marcelocantos marcelocantos mentioned this pull request Feb 26, 2018
Closed
avelino
avelino approved these changes Feb 28, 2018
View reviewed changes
Copy link
Contributor

@avelino avelino left a comment

Choose a reason for hiding this comment

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

LGTM

@gopherbot
Copy link

Message from Andrew Bonventre:

Patch Set 1:

(1 comment)

This should have a test added that fails with the old behavior and succeeds with this fix.

Please don’t reply on this GitHub thread. Visit golang.org/cl/97256.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link

This PR (HEAD: 738b20a) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/#/c/lint/+/97256 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link

Message from Marcelo Cantos:

Patch Set 2:

Patch Set 1:

(1 comment)

This should have a test added that fails with the old behavior and succeeds with this fix.

Done

Please don’t reply on this GitHub thread. Visit golang.org/cl/97256.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link

Message from Andrew Bonventre:

Patch Set 2:

(1 comment)

Please don’t reply on this GitHub thread. Visit golang.org/cl/97256.
After addressing review feedback, remember to publish your drafts!

@marcelocantos marcelocantos changed the title Swap IfStmt tests to not skip every other IfStmt in a chain lint: Swap IfStmt tests to not skip every other IfStmt in a chain Mar 1, 2018
@gopherbot
Copy link

Message from Gerrit Bot:

Uploaded patch set 3: Commit message was updated.

Please don’t reply on this GitHub thread. Visit golang.org/cl/97256.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link

Message from Gerrit Bot:

Uploaded patch set 4: Commit message was updated.

Please don’t reply on this GitHub thread. Visit golang.org/cl/97256.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link

Message from Andrew Bonventre:

Patch Set 4: Code-Review+1

(3 comments)

Please don’t reply on this GitHub thread. Visit golang.org/cl/97256.
After addressing review feedback, remember to publish your drafts!

@marcelocantos marcelocantos changed the title lint: Swap IfStmt tests to not skip every other IfStmt in a chain lint: swap IfStmt tests to not skip every other IfStmt in a chain Mar 2, 2018
@gopherbot
Copy link

Message from Gerrit Bot:

Uploaded patch set 5: Commit message was updated.

Please don’t reply on this GitHub thread. Visit golang.org/cl/97256.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link

Message from Andrew Bonventre:

Patch Set 5:

(1 comment)

Please remember to respond to the comments in Gerrit and mark them as resolved as appropriate. This helps us determine what still needs to be done.

Also, remember to publish your drafts via the reply button.

Please don’t reply on this GitHub thread. Visit golang.org/cl/97256.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link

Message from Marcelo Cantos:

Patch Set 5:

(4 comments)

Patch Set 2:

(1 comment)

All done, I think.

Please don’t reply on this GitHub thread. Visit golang.org/cl/97256.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link

Message from Andrew Bonventre:

Patch Set 5:

ping alan

Please don’t reply on this GitHub thread. Visit golang.org/cl/97256.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link

Message from Alan Donovan:

Patch Set 5: Code-Review+2

Please don’t reply on this GitHub thread. Visit golang.org/cl/97256.
After addressing review feedback, remember to publish your drafts!

gopherbot pushed a commit that referenced this pull request Mar 19, 2018
@marcelocantos @andybons
lint: swap IfStmt tests to not skip every other IfStmt in a chain
85993ff 
Fixes #382

The fix works by moving the test for ignored nodes after the else-node check.

Change-Id: I508c65ec0b49409a5a7340b5fa5ccc1ccd4a4b05
GitHub-Last-Rev: 738b20a
GitHub-Pull-Request: #383
Reviewed-on: https://go-review.googlesource.com/97256
Reviewed-by: Alan Donovan <adonovan@google.com>
@gopherbot
Copy link

This PR is being closed because golang.org/cl/97256 has been merged.

@gopherbot gopherbot closed this Mar 19, 2018
@marcelocantos marcelocantos deleted the fix-ifstmt-return-check branch November 30, 2019 23:17
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
1 more reviewer

@avelino avelino avelino approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Spurious 'if block ends with a return statement…' when some preceding if-else clauses don't return unconditionally.
4 participants
@marcelocantos @gopherbot @avelino @googlebot