-
Notifications
You must be signed in to change notification settings - Fork 178
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 clippy warning and enforce no warning in the CI #273
Conversation
@chevdor I took the liberty to merge in master. |
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.
LGTM modulo -D warnings
in the CI
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.
not be merged lightly as it would require a major version bump.
dq: is this a breaking change because we now return an error in a logic branch that previously would have not error'ed? Since the return type doesn't change on try_merge
could it be considered just a patch?
I would regard it as patch/bug fix but for now we don't care about about breaking changes because we are still at |
@emostov in theory yes, with the last change I made, a fn has been changed. But if you treated already the |
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 @chevdor & @niklasad1 for the quick answer. From my PoV this looks good
@chevdor feel free to merge when CI clears, and ty! :) |
This PR fixes a (the single) clippy warning and enforces in the CI that warnings are unwanted.
This PR is small/short but should not be merged lightly as it would require a major version bump.
It is also not critical to have immediately so it would be a good candidate to pack with other PRs that would change the public interface.