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

Ignore global error variables (implements #2) #3

Merged
merged 4 commits into from
Sep 8, 2018

Conversation

leonklingele
Copy link
Contributor

@leonklingele leonklingele commented Sep 6, 2018

Error variables are variables starting with 'err' or 'Err' which
were made by either errors.New or fmt.Errorf.

There still is leeway for improvement such as checking if the
node implements the error interface. This would allow us to
whitelist custom error variables.

Fixes #2

@codecov-io
Copy link

codecov-io commented Sep 6, 2018

Codecov Report

Merging #3 into master will increase coverage by 4.46%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master       #3      +/-   ##
==========================================
+ Coverage   54.68%   59.15%   +4.46%     
==========================================
  Files           2        2              
  Lines          64       71       +7     
==========================================
+ Hits           35       42       +7     
  Misses         27       27              
  Partials        2        2
Impacted Files Coverage Δ
check_no_globals.go 91.3% <100%> (+1.56%) ⬆️

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 9d4b45f...3f4f961. Read the comment docs.

Copy link
Owner

@leighmcculloch leighmcculloch left a comment

Choose a reason for hiding this comment

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

@leonklingele Thanks for submitting this so promptly! Thoughts below, let me know what you think.

check_no_globals.go Outdated Show resolved Hide resolved
check_no_globals.go Outdated Show resolved Hide resolved
testdata/7/code.go Outdated Show resolved Hide resolved
testdata/7/code.go Outdated Show resolved Hide resolved
@leonklingele
Copy link
Contributor Author

Updated to also catch customly-typed error variables, "fake errors" will no longer be caught.
PTAL @leighmcculloch :)

testdata/7/code.go Outdated Show resolved Hide resolved
Copy link
Owner

@leighmcculloch leighmcculloch left a comment

Choose a reason for hiding this comment

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

This is so exciting.

check_no_globals.go Outdated Show resolved Hide resolved
Error variables are variables starting with 'err' or 'Err'.

This implements leighmcculloch#2.
@leonklingele
Copy link
Contributor Author

This should be fine now @leighmcculloch :)

@leighmcculloch
Copy link
Owner

@leonklingele 👍, is 3f4f961 better?

@leonklingele
Copy link
Contributor Author

lgtm! :) Thanks for the improvements.

@leighmcculloch leighmcculloch merged commit c6d36d2 into leighmcculloch:master Sep 8, 2018
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