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

Addressed clippy warnings from clippy 0.1.67 (ec56537c 2022-12-15) #1717

Merged
merged 4 commits into from
Dec 22, 2022

Conversation

UebelAndre
Copy link
Collaborator

This PR addresses newer clippy warnings that exist in nightly toolchains. #1712 introduces regression testing for this change but since it's larger change I thought it made sense to review separately.

Copy link
Collaborator

@illicitonion illicitonion left a comment

Choose a reason for hiding this comment

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

I think this is generally reasonable, but, it does restrict rules_rust support to only rust versions 1.58.0 (released January 2022) and newer. Specifically because the process wrapper will now be using a construct only supported since that release.

If we care about supporting older rusts, we should probably disable this particular lint in the process_wrapper and revert the changes in that one binary.

I personally am fine with dropping support for year-old rusts, but also if someone else does care, I don't think slightly-cleaner-format-strings is a valuable enough feature to justify breaking support for older rusts.

@UebelAndre
Copy link
Collaborator Author

I think this is generally reasonable, but, it does restrict rules_rust support to only rust versions 1.58.0 (released January 2022) and newer. Specifically because the process wrapper will now be using a construct only supported since that release.

If we care about supporting older rusts, we should probably disable this particular lint in the process_wrapper and revert the changes in that one binary.

I personally am fine with dropping support for year-old rusts, but also if someone else does care, I don't think slightly-cleaner-format-strings is a valuable enough feature to justify breaking support for older rusts.

I would agree some formatting fixes would be an upsetting thing to change the min supported version for. However, before making that change I wanted to know what the min version there repo supported was so I opened #1720 and found issues on versions less than 1.59.0. I only stopped at one and haven't looked to see if there's a work-around. I posted the error on the following comment and think we should discuss that there: #1720 (comment)

@illicitonion
Copy link
Collaborator

Works for me - ship it!

@UebelAndre UebelAndre merged commit 1357b85 into bazelbuild:main Dec 22, 2022
@UebelAndre UebelAndre deleted the clippy branch December 22, 2022 17:07
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.

2 participants