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

improve the suggestion of the lint unit-arg #5931

Merged
merged 4 commits into from
Sep 10, 2020
Merged

Conversation

tnielens
Copy link
Contributor

@tnielens tnielens commented Aug 20, 2020

Fixes #5823
Fixes #6015

Changes

help: move the expression in front of the call...
  |
3 |     g();
  |
help: ...and use a unit literal instead
  |
3 |     o.map_or((), |i| f(i))
  | 

into

help: move the expression in front of the call and replace it with the unit literal `()`
  |
3 |     g();
  |     o.map_or((), |i| f(i))
  |

changelog: improve the suggestion of the lint unit-arg

@rust-highfive
Copy link

r? @phansch

(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 Aug 20, 2020
@flip1995
Copy link
Member

r? @flip1995

Since I produced the latest (problematic) suggestion.

@rust-highfive rust-highfive assigned flip1995 and unassigned phansch Aug 24, 2020
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.

Thanks for further improving the suggestion! Only one case is indented weirdly now.

Comment on lines 127 to 131
LL | foo(0);
LL | foo(1);
LL | }; {
LL | foo(2);
LL | foo(3);
Copy link
Member

Choose a reason for hiding this comment

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

This looks weird

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is still an indentation issue I must handle.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

tests/ui/unit_arg.stderr Outdated Show resolved Hide resolved
tests/ui/unit_arg.rs Show resolved Hide resolved
@flip1995 flip1995 added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Aug 24, 2020
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.

LGTM overall, except one code-style thing.

Comment on lines 869 to 887
let wrap_in_block =
!matches!(parent_node, Some(Node::Block(_))) && !matches!(parent_node, Some(Node::Stmt(_)));

let stmts_indent = if wrap_in_block { indent + 4 } else { indent };
let mut stmts_and_call = arg_snippets_without_empty_blocks.clone();
stmts_and_call.push(expr_with_replacements);
let mut stmts_and_call_str = stmts_and_call
.into_iter()
.enumerate()
.map(|(i, v)| {
let with_indent_prefix = if i > 0 { " ".repeat(stmts_indent) + &v } else { v };
trim_multiline(with_indent_prefix.into(), true, Some(stmts_indent)).into_owned()
})
.collect::<Vec<String>>()
.join(";\n");

if wrap_in_block {
stmts_and_call_str = " ".repeat(stmts_indent) + &stmts_and_call_str;
stmts_and_call_str = format!("{{\n{}\n{}}}", &stmts_and_call_str, " ".repeat(indent));
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to wrap this wrap_in_block handling code into its own function to get rid of the allow above?

Copy link
Contributor Author

@tnielens tnielens Sep 2, 2020

Choose a reason for hiding this comment

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

I moved that logic out. That clarifies the code a bit 👍.
I had to modify and rename the trim_multiline utils method to let it indent code further as it only supported dedentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in case you missed the notif @flip1995 (sorry if have no time atm)

@flip1995
Copy link
Member

@bors r+

Thanks! And sorry for the late reply

@bors
Copy link
Collaborator

bors commented Sep 10, 2020

📌 Commit b220ddf has been approved by flip1995

@bors
Copy link
Collaborator

bors commented Sep 10, 2020

⌛ Testing commit b220ddf with merge 872e91e...

bors added a commit that referenced this pull request Sep 10, 2020
improve the suggestion of the lint `unit-arg`

Fixes #5823
Fixes #6015

Changes
```
help: move the expression in front of the call...
  |
3 |     g();
  |
help: ...and use a unit literal instead
  |
3 |     o.map_or((), |i| f(i))
  |
```
into
```
help: move the expression in front of the call and replace it with the unit literal `()`
  |
3 |     g();
  |     o.map_or((), |i| f(i))
  |
```
changelog: improve the suggestion of the lint `unit-arg`
@bors
Copy link
Collaborator

bors commented Sep 10, 2020

💔 Test failed - checks-action_test

@flip1995 flip1995 added S-waiting-on-bors Status: The marked PR was approved and is only waiting bors and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Sep 10, 2020
@flip1995
Copy link
Member

@bors retry

@bors
Copy link
Collaborator

bors commented Sep 10, 2020

⌛ Testing commit b220ddf with merge 55efa96...

@bors
Copy link
Collaborator

bors commented Sep 10, 2020

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

@bors bors merged commit 55efa96 into rust-lang:master Sep 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: The marked PR was approved and is only waiting bors
Projects
None yet
5 participants