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

fallout: fix tests to allow uninlined_format_args #9547

Merged
merged 1 commit into from
Oct 4, 2022

Conversation

nyurik
Copy link
Contributor

@nyurik nyurik commented Sep 27, 2022

In order to switch clippy::uninlined_format_args from pedantic to style, all existing tests must not raise a warning. I did not want to change the actual tests, so this is a relatively minor change that:

  • add #![allow(clippy::uninlined_format_args)] where needed
  • normalizes all allow/deny/warn attributes
    • all allow attributes are grouped together
    • sorted alphabetically
    • the clippy::* attributes are listed separate from the other ones.
    • deny and warn attributes are listed before the allowed ones

See also #9233, #9525, #9527

cc: @llogiq @Alexendoo

changelog: none

@rust-highfive
Copy link

r? @flip1995

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Sep 27, 2022
@bors
Copy link
Contributor

bors commented Sep 29, 2022

☔ The latest upstream changes (presumably #9516) made this pull request unmergeable. Please resolve the merge conflicts.

@nyurik nyurik changed the title fallout: allow new uninlined_format_args in tests fallout: fix tests to allow uninlined_format_args Sep 29, 2022
Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

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

This adds an allow attribute for the uninlined_format_args lint for every test file, even if it doesn't even use the format! macro. This is highly unnecessary and not something we want. Please only add this attribute to the tests that would actually fail due to the new lint.

@nyurik
Copy link
Contributor Author

nyurik commented Oct 1, 2022

This adds an allow attribute for the uninlined_format_args lint for every test file, even if it doesn't even use the format! macro. This is highly unnecessary and not something we want. Please only add this attribute to the tests that would actually fail due to the new lint.

@flip1995 that is not correct - there are 801 test .rs files in the tests/ui dir, and I only changed 86 of them - only about 10%, all of which are required. I reevaluated all of them again, and did find one -- tests/ui/empty_line_after_outer_attribute.rs that I refactored by accident (reverted). Just to keep things focused, I also undeed the moveable -> movable rename. What's left are all needed.

@nyurik nyurik requested a review from flip1995 October 1, 2022 01:29
@bors
Copy link
Contributor

bors commented Oct 2, 2022

☔ The latest upstream changes (presumably #9479) made this pull request unmergeable. Please resolve the merge conflicts.

In order to switch `clippy::uninlined_format_args` from pedantic to
style, all existing tests must not raise a warning. I did not want to
change the actual tests, so this is a relatively minor change that:

* add `#![allow(clippy::uninlined_format_args)]` where needed
* normalizes all allow/deny/warn attributes
   * all allow attributes are grouped together
   * sorted alphabetically
   * the `clippy::*` attributes are listed separate from the other ones.
   * deny and warn attributes are listed before the allowed ones

changelog: none
@nyurik
Copy link
Contributor Author

nyurik commented Oct 4, 2022

@flip1995 is there still something that needs to be done to this PR? Thx!

Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

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

Sorry, I was away over the weekend (long weekend in Germany).

All good now. Maybe I saw ghosts, but I thought I saw a file where it wasn't necessary. Thanks for addressing this!

@flip1995
Copy link
Member

flip1995 commented Oct 4, 2022

@bors r+ p=10

@bors
Copy link
Contributor

bors commented Oct 4, 2022

📌 Commit eb39702 has been approved by flip1995

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Oct 4, 2022

⌛ Testing commit eb39702 with merge 2f90b2a...

@bors
Copy link
Contributor

bors commented Oct 4, 2022

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: flip1995
Pushing 2f90b2a to master...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants