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.

Sign up for GitHub

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

Jump to bottom

[semicolon_if_nothing_returned]: add an autofix #11083

Merged
merged 2 commits into from Jul 17, 2023
Merged

[semicolon_if_nothing_returned]: add an autofix #11083

merged 2 commits into from Jul 17, 2023

Conversation

sylvestre
Copy link
Contributor

@sylvestre sylvestre commented Jul 3, 2023 •
edited

changelog: [semicolon_if_nothing_returned]: add an autofix

@rustbot
Copy link
Collaborator

rustbot commented Jul 3, 2023

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @dswij (or someone else) soon.

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 Jul 3, 2023
dswij
dswij reviewed Jul 4, 2023
View reviewed changes
Copy link
Member

@dswij dswij left a comment

Choose a reason for hiding this comment

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

Thank you.

Can you also help to add the test for autofix?

You'll need to add //@run-rustfix header at the top of tests/ui/semicolon_if_nothing_returned.rs.

A .fixed file will be generated that will result in the test failure. If nothing is weird, you'll need to run the test again with --bless to copy the generated file as the expected output.

@sylvestre
Copy link
Contributor Author

@dswij done, sorry it took that long :)

@dswij
Copy link
Member

dswij commented Jul 14, 2023

@dswij done, sorry it took that long :)

No worries at all. Thank you for taking the time to add the test!

@bors r+

@bors
Copy link
Collaborator

bors commented Jul 14, 2023

📌 Commit 09f8634 has been approved by dswij

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Jul 14, 2023

⌛ Testing commit 09f8634 with merge 8882969...

bors added a commit that referenced this pull request Jul 14, 2023
@bors
Auto merge of #11083 - sylvestre:autofix, r=dswij
8882969 
[`semicolon_if_nothing_returned`]: add an autofix

changelog:
[`semicolon_if_nothing_returned`]: add an autofix
@bors
Copy link
Collaborator

bors commented Jul 14, 2023

💔 Test failed - checks-action_test

@sylvestre
Copy link
Contributor Author

@dswij
I guess it failed because it was
changelog:
fxx

I updated the initial PR comment to be on only one line

@dswij
Copy link
Member

dswij commented Jul 15, 2023

@bors retry

@bors
Copy link
Collaborator

bors commented Jul 15, 2023

⌛ Testing commit 09f8634 with merge 93defe5...

bors added a commit that referenced this pull request Jul 15, 2023
@bors
Auto merge of #11083 - sylvestre:autofix, r=dswij
93defe5 
[`semicolon_if_nothing_returned`]: add an autofix

changelog: [`semicolon_if_nothing_returned`]: add an autofix
@bors
Copy link
Collaborator

bors commented Jul 15, 2023

💔 Test failed - checks-action_test

Centri3
Centri3 reviewed Jul 16, 2023
View reviewed changes
tests/ui/semicolon_if_nothing_returned.fixed Outdated

// No warning
fn let_else_stmts() {
let Some(x) = function_returning_option() else { return; };
Copy link
Member

@Centri3 Centri3 Jul 16, 2023 •
edited

Choose a reason for hiding this comment

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

Seems you'll need to run cargo dev fmt.

let..else is now, (finally!), formatted. That seems to be the only issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Centri3
are you sure? cargo dev fmt doesn't make any change on my system
with
rustfmt 1.5.3-nightly (5bd28f5 2023-06-28)

Copy link
Member

@Centri3 Centri3 Jul 16, 2023 •
edited

Choose a reason for hiding this comment

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

2023-06-28 is pretty old, current rust-toolchain shows 2023-07-14 and rustfmt -V rustfmt 1.6.0-nightly (7bd81ee 2023-07-13) so you'll likely want to git pull upstream master

Copy link
Contributor Author

Choose a reason for hiding this comment

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

my bad, rustup didn't updated to this one ?!
should be good now!

@sylvestre
Copy link
Contributor Author

@bors retry
(in case I have permissions)

@bors
Copy link
Collaborator

bors commented Jul 17, 2023

@sylvestre: 🔑 Insufficient privileges: not in try users

@dswij
Copy link
Member

dswij commented Jul 17, 2023

@bors r+

@bors
Copy link
Collaborator

bors commented Jul 17, 2023

📌 Commit 8ebdd62 has been approved by dswij

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Jul 17, 2023

⌛ Testing commit 8ebdd62 with merge 568ccf3...

@bors
Copy link
Collaborator

bors commented Jul 17, 2023

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: dswij
Pushing 568ccf3 to master...

@bors bors merged commit 568ccf3 into rust-lang:master Jul 17, 2023
5 checks passed
@sylvestre sylvestre deleted the autofix branch July 17, 2023 08:45
@dswij
Copy link
Member

dswij commented Jul 17, 2023

@sylvestre Thank you for this and @Centri3 for helping

Apologies for the slow response at times and the hurdles in merging the PR 😕

@sylvestre
Copy link
Contributor Author

ouhlala, don't apologize :) this was perfect!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Centri3 Centri3 Centri3 left review comments

@dswij dswij dswij approved these changes

Assignees

@dswij dswij

Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@sylvestre @rustbot @dswij @bors @Centri3