-
-
Notifications
You must be signed in to change notification settings - Fork 109
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
fix(golangci-lint): fix errors as raised by linter #323
Conversation
thanks @boreyuk appreciate the contribution and response on the issue. I've tagged Matt and let the PR run. I've also updated the release pipeline to 1.20 so this linter probably wants to run against that as well. (I think we can probably drop 1.17/1.18 as they are unsupported for go, but that probably warrants another PR) |
Oops, seems like the CI fails in the 1.19.x env 😅
I'm trying to check if this is related to some code I modified 😅 |
This relates to #269, I think. We have yet to chase down that nasty bug. |
This is great, thank you. I think for the test files and the unused functions, I'll take those as is. For the real ones where we swallow the errors, perhaps we actually need to |
I agree - i think that it's better to explicitely panic than silently fail 😛 |
thanks for updated commit @boreyuk have kicked off the workflow now |
Thanks @YOU54F ! After checking, the tests are currently failing due to the Do you prefer to keep the panic and modify the test, or should I remove the panic() to keep the same behavior than before? |
So that is a plugin specific error (the plugins themselves define the content make up of their messages, and can choose to error) I think that error will be coming from the underlying plugin we are calling. As we can't proceed in that case, and we need to perform the action in the message provided, it would make sense for this to be an error for the user. the plugins communicate with the central pact framework via grpc so there is room for error here for a number of variables. Having this swallowed here I think would be disadvantageous all round, so given that I would suggest a panic (provided it that also give us that detail) Thanks for having a look again my friend, really do appreciate it |
Thanks for your feedback ! I just modified the test in order to make it pass, and I commited before seeing your comment 😅 |
No worries @boreyuk I'll let @mefellows make the final call, as he is the core maintainer of the repo. |
I think we should drop 1.17 due it being out of support is there value is testing across the different supported versions of go with golanglint-ci or just the latest version we use to build/release |
I think we need to skip linting of the C directive in either way it works, once built so I assume this is a bit of a red herring? |
I added a commit which modifies the golangci-lint in order to ignore the |
😄 s'all good, nothing like a bit of experimentation 👍🏾 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome!
Man it will be nice to be able to see green in the pipeline again. Thanks dude
This is great, thanks. Those additional TODOs are also good food for thought, but we can pick them up in the future. |
Nice ! Thanks to you two 🎉 |
You're so welcome, we really appreciate it. If you're up for me, I'd love to chat with you (just hit me up in the #pact-go channel in slack.pact.io). |
Linked to #320