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

fix suggestion on [map_flatten] being cropped causing possible information loss #8520

Merged
merged 1 commit into from
Mar 21, 2022
Merged

fix suggestion on [map_flatten] being cropped causing possible information loss #8520

merged 1 commit into from
Mar 21, 2022

Conversation

J-ZhengLi
Copy link
Member

@J-ZhengLi J-ZhengLi commented Mar 11, 2022 •
edited
Loading

fixes #8506

Multi-line suggestion given by the lint is missing its bottom part, which could potentially contains useful information about the fix.

changelog: [map_flatten]: Long suggestions will now be splitup into two help messages

@rust-highfive
Copy link

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

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Mar 11, 2022
@J-ZhengLi
Copy link
Member Author

J-ZhengLi commented Mar 11, 2022 •
edited
Loading

@xFrednet Hi, I wasn't sure if I understand your suggestion throughly, so I did what I could and get stuck with not knowing how to modify Span as well to reflect the modified suggestion string 😆 was I even in the right direction?

@J-ZhengLi J-ZhengLi changed the title [WIP] fix suggestion being cropped causing lose of infomation [WIP] fix suggestion being cropped causing possible information loss Mar 11, 2022
xFrednet
xFrednet reviewed Mar 11, 2022
View reviewed changes
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.

@xFrednet Hi, I wasn't sure if I understand your suggestion throughly, so I did what I could and get stuck with not knowing how to modify Span as well to reflect the modified suggestion string laughing was I even in the right direction?

The direction is good, I underestimated the effort it can take to span over the first and last three lines. I suggested a solution and added some sources where you can search, if you want to create that suggestion 🙃

clippy_lints/src/methods/map_flatten.rs Outdated Show resolved Hide resolved
clippy_lints/src/methods/map_flatten.rs Outdated Show resolved Hide resolved
@J-ZhengLi
Copy link
Member Author

@xFrednet Hi, I wasn't sure if I understand your suggestion throughly, so I did what I could and get stuck with not knowing how to modify Span as well to reflect the modified suggestion string laughing was I even in the right direction?

The direction is good, I underestimated the effort it can take to span over the first and last three lines. I suggested a solution and added some sources where you can search, if you want to create that suggestion 🙃

Thank you~

@J-ZhengLi J-ZhengLi requested a review from xFrednet March 14, 2022 10:48
@J-ZhengLi
Copy link
Member Author

Thank you for your suggestion again @xFrednet , so far it looks alright, but I just want to wait for some feedback before I proceed to covering check for Iterators and do some code fractoring.

xFrednet
xFrednet reviewed Mar 14, 2022
View reviewed changes
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.

You found a very nice solution to span over the surrounding lines. I learned something from this review 👍. I left some comments which should be addressed, but the current version is already a good step, thank you for the work 🙃

clippy_lints/src/methods/map_flatten.rs Outdated Show resolved Hide resolved
clippy_lints/src/methods/map_flatten.rs Outdated Show resolved Hide resolved
clippy_lints/src/methods/map_flatten.rs Outdated Show resolved Hide resolved
@J-ZhengLi J-ZhengLi changed the title [WIP] fix suggestion being cropped causing possible information loss fix suggestion on [map_flatten] being cropped causing possible information loss Mar 15, 2022
@J-ZhengLi J-ZhengLi mentioned this pull request Mar 16, 2022
Open
xFrednet
xFrednet reviewed Mar 17, 2022
View reviewed changes
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.

Excellent work. I added some smaller notes that should be easy to fix, and then it's ready to be merged! 🙃

clippy_lints/src/methods/map_flatten.rs Outdated Show resolved Hide resolved
clippy_lints/src/methods/map_flatten.rs Outdated Show resolved Hide resolved
clippy_lints/src/methods/map_flatten.rs Outdated Show resolved Hide resolved
clippy_lints/src/methods/map_flatten.rs Outdated Show resolved Hide resolved
xFrednet
xFrednet reviewed Mar 20, 2022
View reviewed changes
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 is almost ready to be merged. I'm very pleased with the output, it looks excellent.

Besides the comment, there is one more thing. Clippy follows rustc's no merge-commit policy. The merge commit should be removed before this can be merged into our master. You can do this by dropping the commit and rebasing. You're welcome to ask if you need help with that.

Another small optional NIT. I would change the changelog entry to:

changelog: [`map_flatten`]: Long suggestions will now be splitup into two help messages

clippy_lints/src/utils/internal_lints/metadata_collector.rs Show resolved Hide resolved
clippy_utils/src/diagnostics.rs Outdated Show resolved Hide resolved
clippy_utils/src/diagnostics.rs Show resolved Hide resolved
@J-ZhengLi J-ZhengLi force-pushed the issue8506 branch 12 times, most recently from 35f7c45 to b64ecc0 Compare March 21, 2022 03:44
@J-ZhengLi J-ZhengLi closed this Mar 21, 2022
@J-ZhengLi J-ZhengLi reopened this Mar 21, 2022
@J-ZhengLi
allowing [map_flatten] to split long suggestions
5b6295d 
add new function `span_lint_and_sugg_` for edges in `clippy_utils::diagnostics`
@xFrednet
Copy link
Member

Looks good to me, thank you for the work! I hope you also had fun while working on this. 🙃

@bors r+

@bors
Copy link
Contributor

bors commented Mar 21, 2022

📌 Commit 5b6295d has been approved by xFrednet

@bors
Copy link
Contributor

bors commented Mar 21, 2022

⌛ Testing commit 5b6295d with merge ff0a368...

@bors
Copy link
Contributor

bors commented Mar 21, 2022

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

@bors bors merged commit ff0a368 into rust-lang:master Mar 21, 2022
@J-ZhengLi J-ZhengLi deleted the issue8506 branch March 22, 2022 00:00
@Serial-ATA Serial-ATA mentioned this pull request Apr 22, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dswij dswij dswij left review comments

@xFrednet xFrednet xFrednet approved these changes

Assignees

@xFrednet xFrednet

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.

clippy::map_flatten multiline suggestion is missing closing tokens
5 participants
@J-ZhengLi @rust-highfive @xFrednet @bors @dswij