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

dev: refactor regexp usage #24

Merged
merged 6 commits into from Feb 17, 2022
Merged

dev: refactor regexp usage #24

merged 6 commits into from Feb 17, 2022

Conversation

butuzov
Copy link
Contributor

@butuzov butuzov commented Feb 4, 2022

This PR intends to bring the next features/fixes to wrapcheck:

  1. avoid compiling regexp rules into regexp.Regexp for checking each error returning function.
  2. removing os.Exit(1) in case fail to compile regexp rule from the codebase ( we can't handle such cases in golangci-lint: output is hidden, so no log messages received, which ends in no issues or errors comes from wrapcheck if bad regexp provided).

P.S. The rest of the coding is changed by gofumpt automatically.
P.P.S. It would be also great to release fix changes asap (so we update golangci-lint).

Copy link
Owner

@tomarrell tomarrell left a comment

Choose a reason for hiding this comment

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

I would also ask that you include test cases testing for the functionality that has changed in order to confirm that the issue is resolved.

wrapcheck/wrapcheck.go Outdated Show resolved Hide resolved
wrapcheck/wrapcheck.go Outdated Show resolved Hide resolved
@butuzov
Copy link
Contributor Author

butuzov commented Feb 4, 2022

I would also ask that you include test cases testing for the functionality that has changed in order to confirm that the issue is resolved.

let me think about this, analysistest isn't designed to test linter errors.

@butuzov
Copy link
Contributor Author

butuzov commented Feb 5, 2022

@tomarrell I have added a test case and changed the way it passes compiled regexes around. Can you check if this solution suites you?

@butuzov
Copy link
Contributor Author

butuzov commented Feb 14, 2022

ping @tomarrell, do my explanations and fixes make sense?

@tomarrell
Copy link
Owner

Thanks for the ping @butuzov, I'll take a look and let you know.

@tomarrell
Copy link
Owner

@butuzov Did you maybe forget to check in the .wrapcheck.yaml file used as testdata?

@butuzov
Copy link
Contributor Author

butuzov commented Feb 14, 2022

you are right! I forgot to commit it! now its fixed.

@tomarrell
Copy link
Owner

Thanks for that. There's a couple of things I would like to check locally, however I've just started a vacation, but plan to finish this review in the next day or two.

I appreciate your patience in the meantime.

@butuzov
Copy link
Contributor Author

butuzov commented Feb 17, 2022

No problem. This can wait.

Copy link
Owner

@tomarrell tomarrell left a comment

Choose a reason for hiding this comment

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

Looks good. Just made some minor changes, added a more generic way to skip the analysistest run so they can be tested manually.

Thanks for the contribution!

@tomarrell tomarrell merged commit fab0c41 into tomarrell:master Feb 17, 2022
@butuzov butuzov deleted the regexps branch February 19, 2022 04:13
@butuzov
Copy link
Contributor Author

butuzov commented Feb 19, 2022

thank you!

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.

None yet

2 participants