-
Notifications
You must be signed in to change notification settings - Fork 127
Improve code quality based on linter hints #563
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
Conversation
Changes to remove magic constants, handle errors and clean up. Some changes requested by opinionated editor/linter. Makefile fixed to ensure that build directory exists before starting work.
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
🤖 GitHub commentsTo re-run your PR in the CI, just comment with:
|
internal/files/compress.go
Outdated
| os.MkdirAll(workDir, 0755) | ||
| err = os.MkdirAll(workDir, 0755) | ||
| if err != nil { | ||
| return errors.Wrap(err, "can't prepare work directory") |
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.
nit: errors.Wrapf( ..., workDir)?
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.
That will be in the underlying os.PathError, but done. pkg/errors makes me sad.
Makefile
Outdated
|
|
||
| test-go: | ||
| test-build-dir: | ||
| mkdir -p $(PWD)/build/test-results |
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.
That's funny as @stuartnelson3 found it in the same time here :)
I would ask you or @stuartnelson3 to extract changes in Makefile to a separate PR.
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.
Ok, @stuartnelson3 did this already (thanks!). Would you mind adjusting your PR?
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.
Done.
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.
Thanks! Let merge this on
Changes to remove magic constants, handle errors and clean up.
Some changes requested by opinionated editor/linter.
Makefile fixed to ensure that build directory exists before starting work.
Please take a look.