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

Removed @no-rustfix annotation and updated suggestions to multipart_suggestion as required by #13099 #13216

Closed
wants to merge 1 commit into from

Conversation

ibilalkayy
Copy link

Hey there, I removed the @no-rustfix annotation and updated suggestions to multipart_suggestion line from the files that were mentioned in the #13099 issue. I removed the @no-rustfix annotation and mentioned it in the PR also but I didn't understand the 3rd point because I didn't saw any suggestion point except these comments.

Let me know if I am missing anything.

Although there was a test error in the fmt but I didn't modified the tests/check-fmt.rs file. Here is the screenshot of it.

53

  • Followed lint naming conventions
  • Added passing UI tests (including committed .stderr file)
  • cargo test passes locally
  • Executed cargo dev update_lints
  • Added lint documentation
  • Run cargo dev fmt

Note that you can skip the above if you are just opening a WIP PR in
order to get feedback.

Delete this line and everything above before opening your PR.


Please write a short comment explaining your change (or "none" for internal only changes)

changelog:

@rustbot
Copy link
Collaborator

rustbot commented Aug 4, 2024

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @llogiq (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Aug 4, 2024
@llogiq
Copy link
Contributor

llogiq commented Aug 4, 2024

The fmt test is now successful, but some UI tests fail. Running cargo dev bless should make them pass, but I'm not entirely sure that this is the right thing here, I'd have to look into it.

@ibilalkayy
Copy link
Author

ibilalkayy commented Aug 5, 2024

@llogiq I have run cargo dev bless but it is giving me an error present in the screenshot. Also if look into the PR, please let me know if there is any improvement required. I would like to improve.

54

@flip1995
Copy link
Member

flip1995 commented Aug 5, 2024

but I didn't understand the 3rd point because I didn't saw any suggestion point except these comments.

You will have to adapt the lint implementation(s) to use a different style of creating those lint suggestions, so that they can be applied to the test files automatically. I would be really surprised if just removing the annotations will magically work, because then we wouldn't have added them in the first place.

@Alexendoo
Copy link
Member

The libz error is odd, does it work after a cargo clean and if not what system is this running on?

@ibilalkayy
Copy link
Author

ibilalkayy commented Aug 6, 2024

Even in the master branch, cargo test gives me the above error of fmt

@ibilalkayy
Copy link
Author

@Alexendoo I am running this code on the ubuntu machine.

@Alexendoo
Copy link
Member

What's the output of rustc -Vv ran from the clippy directory?

@ibilalkayy
Copy link
Author

@Alexendoo Here is the screenshot.

57

@Alexendoo
Copy link
Member

Looks like it is a known dependency - rust-lang/rust#74420, I believe you'd want to install zlib1g (the apt package, not a cargo one)

You could mention it in that issue that you ran into this error

bors added a commit that referenced this pull request Aug 12, 2024
Add a test for ice-3717.rs

this PR is a part of #13099.

Based on the changes introduced in #13098 for introduce ui_test, we will update the uitest output.
This is a fix for `ice-3717.rs`.

Although fixes have already been made in #13216, it seems that he is a first-time contributor.
I thought it might be better for him to refer to my PR, so I created it accordingly.

Since this is my first contribution in a while, please let me know if there are any issues or required changes.

changelog:
None

r! `@flip1995`
bors added a commit that referenced this pull request Aug 12, 2024
Add a test for ice-3717.rs

this PR is a part of #13099.

Based on the changes introduced in #13098 for introduce ui_test, we will update the uitest output.
This is a fix for `ice-3717.rs`.

Although fixes have already been made in #13216, it seems that he is a first-time contributor.
I thought it might be better for him to refer to my PR, so I created it accordingly.

Since this is my first contribution in a while, please let me know if there are any issues or required changes.

changelog: none

r! `@flip1995`
@kyoto7250
Copy link
Contributor

@ibilalkayy
Hi, I have created a PR that fixes one of the files in this issue. #13230

In PR #13098, all tests that were expected to split into multipart files have been deleted and set to be skipped using @no-rustfix. Therefore, simply removing the annotation is not sufficient.

You remove @no-rustfix and run cargo uitest locally, the tests should fail. For more details, please refer to the following documentation:

https://github.com/rust-lang/rust-clippy/blob/master/book/src/development/writing_tests.md

In my PR, multiple files were not generated, but I believe that in this issue, it is expected that multiple files will be generated by the multipart suggestion.

@flip1995
Copy link
Member

Triage: Seems like this PR went stale. As it doesn't really fix the issue, I'm closing it. Thanks for the explanation @kyoto7250 , I copied your comment to the tracking issue, as I think that it explains the task at hand way better than what I wrote in the issue description!

@flip1995 flip1995 closed this Sep 23, 2024
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.

6 participants