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

Make derivable_impls machine applicable #9429

Merged
merged 1 commit into from
Sep 13, 2022
Merged

Conversation

kraktus
Copy link
Contributor

@kraktus kraktus commented Sep 5, 2022

changelog: [derivable_impls]: Now machine applicable

@rust-highfive
Copy link

r? @xFrednet

(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 5, 2022
Comment on lines 30 to 34
= help: Remove it
help: and instead derive it
|
LL | #[derive(Default)]
|
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since the Span is 0-length long, the struct XXX part is not shown. I don't think this is a big issue now that it can be fixed automatically, but didn't find how to fix it in case it's a deal breaker.

Copy link
Member

@xFrednet xFrednet left a comment

Choose a reason for hiding this comment

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

This looks good to me, I have a small NIT about the error message, but everything else is perfect 🙃

Sorry, that the review took so long, my week has been very chaotic. Next review should be faster 🙃

clippy_lints/src/derivable_impls.rs Outdated Show resolved Hide resolved
@xFrednet
Copy link
Member

This version looks good to me, but you need to bless the test one more time, after fixing the typo 🙃

@kraktus
Copy link
Contributor Author

kraktus commented Sep 13, 2022

Thank you, I don't know for sure the commit policy on the repo (squash everything in the end/no force push during review/...) so haven't cleaned the history (yet if needed)

@xFrednet
Copy link
Member

We have a no merge commit policy. For the number of commits and squashing, we often go by feel. If the commits are structured well with good message or just one fix commit, we usually just merge it as it is. In this case, it would be cool, if you could squash them into one or two commits.

A small thing about squashing a rebasing is that GitHub doesn't work too well with it. So, after the first review, it's typically better to wait until the end for a rebase if you want to. You can of course also ask the reviewer. With small adjustments like this one, I often squash the change directly to the last commit and force push.

In short, we don't have a set in stone policy ^^. It would be cool if you could clean up the history, and then we can merge it 🙃

@xFrednet
Copy link
Member

Perfect, this looks good to me! Thank you very much for this addition and the swift responses. 🙃

@bors r+

@bors
Copy link
Collaborator

bors commented Sep 13, 2022

📌 Commit 6f13203 has been approved by xFrednet

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Sep 13, 2022

⌛ Testing commit 6f13203 with merge 5564158...

@bors
Copy link
Collaborator

bors commented Sep 13, 2022

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: xFrednet
Pushing 5564158 to master...

@bors bors merged commit 5564158 into rust-lang:master Sep 13, 2022
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.

None yet

4 participants