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

Merge some lints together #5563

Merged
merged 7 commits into from
May 16, 2020
Merged

Merge some lints together #5563

merged 7 commits into from
May 16, 2020

Conversation

ThibsG
Copy link
Contributor

@ThibsG ThibsG commented May 3, 2020

This PR merges following lints:

  • block_in_if_condition_expr and block_in_if_condition_stmtblocks_in_if_conditions
  • option_map_unwrap_or, option_map_unwrap_or_else and result_map_unwrap_or_elsemap_unwrap
  • option_unwrap_used and result_unwrap_usedunwrap_used
  • option_expect_used and result_expect_usedexpect_used
  • wrong_pub_self_convention into wrong_self_convention
  • for_loop_over_option and for_loop_over_resultfor_loops_over_fallibles

Lints that have already been merged since the issue was created:

  • new_without_default and new_without_default_derivenew_without_default

Need more discussion:

  • string_add and string_add_assign: do we agree to merge them or not? Is there something more to do? → not merge finally
  • identity_op and modulo_oneuseless_arithmetic: seems outdated, since modulo_arithmetic has been created.

fixes #1078

changelog: Merging some lints together:

  • block_in_if_condition_expr and block_in_if_condition_stmtblocks_in_if_conditions
  • option_map_unwrap_or, option_map_unwrap_or_else and result_map_unwrap_or_elsemap_unwrap_or
  • option_unwrap_used and result_unwrap_usedunwrap_used
  • option_expect_used and result_expect_usedexpect_used
  • for_loop_over_option and for_loop_over_resultfor_loops_over_fallibles

@ThibsG
Copy link
Contributor Author

ThibsG commented May 3, 2020

Btw, i'm not sure about changelog message, feel free to modify it.

@flip1995 flip1995 added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label May 3, 2020
@flip1995
Copy link
Member

flip1995 commented May 3, 2020

  • block_in_if_condition_expr and block_in_if_condition_stmtblock_in_if_condition

✔️

  • option_map_unwrap_or, option_map_unwrap_or_else and result_map_unwrap_or_elsemap_unwrap

❌ → map_unwrap_or

  • option_unwrap_used and result_unwrap_usedunwrap_used

✔️

  • option_expect_used and result_expect_usedexpect_used

✔️

  • wrong_pub_self_convention into wrong_self_convention

❌ This is intentional, since the first one is a sylte lint and the second one is a restriction lint

  • for_loop_over_option and for_loop_over_resultfor_loop_over_fallible

✔️

Need more discussion:

  • string_add and string_add_assign: do we agree to merge them or not? Is there something more to do?

✔️ And move it to restriction

  • identity_op and modulo_oneuseless_arithmetic: seems outdated, since modulo_arithmetic has been created.

❌ modulo_one is different from modulo_arithmetic (correctness vs restriction) also different intentions. I would also left identity_op separated, but would like some input on others for this one.

cc @rust-lang/clippy

@flip1995 flip1995 added the S-needs-discussion Status: Needs further discussion before merging or work can be started label May 3, 2020
@ThibsG
Copy link
Contributor Author

ThibsG commented May 3, 2020

❌ → map_unwrap_or

Lint renamed.

  • wrong_pub_self_convention into wrong_self_convention

❌ This is intentional, since the first one is a sylte lint and the second one is a restriction lint

Commit deleted, lint is back.

  • string_add and string_add_assign: do we agree to merge them or not? Is there something more to do?

✔️ And move it to restriction

Done

  • identity_op and modulo_oneuseless_arithmetic: seems outdated, since modulo_arithmetic has been created.

❌ modulo_one is different from modulo_arithmetic (correctness vs restriction) also different intentions. I would also left identity_op separated, but would like some input on others for this one.

cc @rust-lang/clippy

No problem let's wait for feedback

@Manishearth
Copy link
Member

@flip1995's suggestions seem good, though i'm a bit wary of the string_add one.

Also we should set these up as renamed lints so the old names still work and we have proper diagnostics.

@flip1995
Copy link
Member

flip1995 commented May 4, 2020

though i'm a bit wary of the string_add one.

Oh I should have read the description of those lints more carefully. I thought one lints x = x + s and the other x += s.

After reading it more carefully I would also keep them as separate lints. @ThibsG can you revert the commit for string_add? Sorry for the back and forth.

@ThibsG
Copy link
Contributor Author

ThibsG commented May 4, 2020

The string_add commit has been dropped and lints are now registered as renamed.

@bors
Copy link
Contributor

bors commented May 14, 2020

☔ The latest upstream changes (presumably #5583) made this pull request unmergeable. Please resolve the merge conflicts.

@ThibsG
Copy link
Contributor Author

ThibsG commented May 15, 2020

Do I need to change something in CHANGELOG.md ?

Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

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

singular -> plural for 2 lint names. Otherwise this LGTM.

In the CHANGELOG.md you have to remove old links to the old names. The offending lines are as follows:

     201:3-201:27  
     285:5-285:27  
     286:5-286:27  
    398:27-398:56  
    398:61-398:90  
    451:25-451:49  
    797:47-797:76  
   1071:3-1071:27  
  1071:32-1071:61  
  1090:33-1090:57  
   1091:3-1091:27  

Just remove the [] around the lint names.

Sorry for not re-reviewing for so long, force pushes to PRs sometimes don't show up in the GH notifications.

clippy_lints/src/block_in_if_condition.rs Outdated Show resolved Hide resolved
clippy_lints/src/loops.rs Outdated Show resolved Hide resolved
@flip1995
Copy link
Member

Thanks!

@bors r+

@bors
Copy link
Contributor

bors commented May 16, 2020

📌 Commit ab87f87 has been approved by flip1995

@flip1995
Copy link
Member

@bors p=1

@bors
Copy link
Contributor

bors commented May 16, 2020

⌛ Testing commit ab87f87 with merge cfd720d...

@bors
Copy link
Contributor

bors commented May 16, 2020

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-needs-discussion Status: Needs further discussion before merging or work can be started 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.

Merge some lints together
4 participants