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

Suggest using Path for comparing extensions #10107

Merged
merged 4 commits into from
Jan 4, 2023

Conversation

tylerjw
Copy link
Contributor

@tylerjw tylerjw commented Dec 21, 2022

fixes #10042

changelog: Sugg: [case_sensitive_file_extension_comparisons]: Now displays a suggestion with Path
#10107

@rustbot
Copy link
Collaborator

rustbot commented Dec 21, 2022

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

Please see the contribution instructions for more information.

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Dec 21, 2022
@tylerjw tylerjw force-pushed the suggest_path branch 3 times, most recently from b604d69 to 4e3e652 Compare December 21, 2022 20:25
@tylerjw tylerjw changed the title Sugggest using Path for comparing extensions Suggest using Path for comparing extensions Dec 21, 2022
@tylerjw tylerjw marked this pull request as draft December 21, 2022 20:34
@tylerjw tylerjw marked this pull request as ready for review December 21, 2022 20:54
@tylerjw tylerjw force-pushed the suggest_path branch 2 times, most recently from 66f2425 to 2c79c4d Compare December 21, 2022 21:05
@tylerjw
Copy link
Contributor Author

tylerjw commented Jan 2, 2023

I'm could use some mentoring on what to do next about this PR.

  1. Should I be using string operations to build the suggestion or using something like fold from rustc? The idea would be to traverse recv and remove method calls to_lowercase and to_uppercase. I looked into doing this and could not hack this together so it would work or find any examples of similar things in clippy. Advice here would be really helpful.

  2. On creating a multi-line suggestion. My current approach seems to hard-code the indentation level. I also have a copy of this that makes this a single-line change, but the result is a diagnostic that runs off the right side of my terminal. Is there tooling for creating multi-line suggestions that have the correct indentation, look good in the terminal output, and are not too far from what rustfmt would like?

@Alexendoo
Copy link
Member

Hi, sorry for taking a while to get to this

For 1. I would say no, removing a trailing .to_lowercase()/.to_uppercase() is the only place it can be done safely without other checks that are overkill since it wouldn't be that common, e.g. in f(s.to_lowercase()).ends_with(".txt") we don't know what's going to happen in f

For 2. you could look at using snippet_indent or reindent_multiline + indent_of

@tylerjw
Copy link
Contributor Author

tylerjw commented Jan 2, 2023

Thank you for the reply. Are you saying I should adjust this PR to only remove the to_lowercase call in the case where it is at the end of the recv code snippet? That makes allot of sense as the only place where it is safe to do this in. Should I still provide a diagnostic about maybe removing it when I detect it in the line but not at the end?

@tylerjw
Copy link
Contributor Author

tylerjw commented Jan 2, 2023

@Alexendoo I updated this PR based on your feedback. Let me know if you would like me to change anything. Thank you again for the links for indenting, that was easy to fixup once I knew about those functions.

I made my new changes a separate commit so it is easier for you to review them. If you'd like me to I'm happy to rebase this PR into a single commit. I don't know what is the norm for this project.

@tylerjw tylerjw force-pushed the suggest_path branch 2 times, most recently from e87eda0 to 3e6f703 Compare January 2, 2023 14:26
@tylerjw
Copy link
Contributor Author

tylerjw commented Jan 4, 2023

I've applied the changes you suggested and tested them. Let me know if I did it correctly and if you'd like any more changes. Thank you for the review.

@Alexendoo
Copy link
Member

Yep looks great thanks!

@bors r+

@bors
Copy link
Collaborator

bors commented Jan 4, 2023

📌 Commit ea6ff7e has been approved by Alexendoo

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Jan 4, 2023

⌛ Testing commit ea6ff7e with merge a385d34...

@bors
Copy link
Collaborator

bors commented Jan 4, 2023

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

@bors bors merged commit a385d34 into rust-lang:master Jan 4, 2023
@tylerjw tylerjw deleted the suggest_path branch January 4, 2023 20:25
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.

case_sensitive_file_extension_comparisons hits on a to_lowercase'd string
4 participants