Conversation
gimalay
left a comment
There was a problem hiding this comment.
I like the changes, lots of minor cleanups. Thanks a lot for the help with this!
I'm not sure about using linter in the CI but it makes sense as pre-commit hook. What would you recommend?
Please see the comments. Make sue that you read and agree with https://github.com/iwe-org/iwe/blob/master/CONTRIBUTING.md and I will merge.
renesat
left a comment
There was a problem hiding this comment.
What We Reject
We will reject pull requests that only contain:
- Formatting or style changes without functional improvements
- Linting fixes unrelated to actual issues
- Typo fixes in code comments or variable names
- Other trivial changes that don't add meaningful value
Yep, I should have read it :)
I'm not sure about using linter in the CI but it makes sense as pre-commit hook. What would you recommend?
You can add it as an optional task in CI, and it won't affect merges.
Pre-commit is a good thing, but first, it can be bypassed and a PR can be made without linters, and second, not everyone likes forced verification at the commit stage. CI cannot be bypassed with PR and does not require additional verification actions for either maintainers or contributors.
|
While it's just a linter fixes, the changes look good to me. Thanks for the contributing! Just to make sure, do you agree with this? Contribution TermsBy submitting a contribution to this project, you agree to the following terms:
|
Yes, of course. Nothing particularly specific, it seems. I wasn't going to revoke the code :) |
|
Thanks for your help, much appreciated! |
Again, a bit of refactoring. I just went through clippy and corrected it. Are you thinking of implementing clippy and rustfmt in CI?