Skip to content

Add SelectorName in UncheckedError#197

Merged
kisielk merged 1 commit into
kisielk:masterfrom
SVilgelm:selector-name
Feb 25, 2021
Merged

Add SelectorName in UncheckedError#197
kisielk merged 1 commit into
kisielk:masterfrom
SVilgelm:selector-name

Conversation

@SVilgelm
Copy link
Copy Markdown

Added a SelectorName field in the UncheckedError struct to store the original func name.

We cannot integrate the errchecker linter with the golangci-lint tool without this changes, we have an additional exclude of os.Stdout|Stderr.Write, but the fullName function returns (*os.File).Write and ignoring os.File is not acceptable.

@SVilgelm
Copy link
Copy Markdown
Author

@kisielk Could you please review this? One more attempt to integrate with golangci-lint

Comment thread errcheck/errcheck.go Outdated
@kisielk
Copy link
Copy Markdown
Owner

kisielk commented Feb 25, 2021

Looks good to me. Would you mind also adding a test to the test suite?

@SVilgelm
Copy link
Copy Markdown
Author

Looks good to me. Would you mind also adding a test to the test suite?

sure, will add

@SVilgelm
Copy link
Copy Markdown
Author

@kisielk I added a very simple test, that the original line should contain the selectorName. Is it good enough?

@kisielk
Copy link
Copy Markdown
Owner

kisielk commented Feb 25, 2021

That is ok for now I think. The tests need a bit of a revamp to look for the specific test data anyway.. Let me check with the other maintainers if this change is ok then we can merge.

@SVilgelm
Copy link
Copy Markdown
Author

Thanks

Copy link
Copy Markdown
Collaborator

@dtcaciuc dtcaciuc left a comment

Choose a reason for hiding this comment

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

Basically what Kamil already said, current check seems more like a smoke test, but doesn't look like we can do much more without involved refactoring. LGTM

@SVilgelm
Copy link
Copy Markdown
Author

we can write a test to test only the selectorName function, but I didn't;t figure out how to properly create the ast Tree to cover all cases. I will try to work on it

@kisielk
Copy link
Copy Markdown
Owner

kisielk commented Feb 25, 2021

@SVilgelm I wouldn't worry about it for now, I don't really want to test un-exported functions, we just need to go through and restructure the existing tests.

@kisielk kisielk merged commit 4174a4a into kisielk:master Feb 25, 2021
@SVilgelm SVilgelm deleted the selector-name branch February 25, 2021 02:49
@SVilgelm
Copy link
Copy Markdown
Author

@kisielk @dtcaciuc thank you! It works fine: golangci/golangci-lint#1319
Could you please release a new version?

@kisielk
Copy link
Copy Markdown
Owner

kisielk commented Feb 25, 2021

I've tagged v1.6.0

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.

3 participants