Adding where_single_line option #2030

afshinm · 2017-10-04T16:52:30Z

Fixes #1936

…e is true

…ngle_line is true

nrc

Thanks for the PR! I think there are few things that could be polished, but otherwise looks good.

nrc · 2017-10-05T12:37:53Z

src/items.rs

@@ -2533,6 +2539,8 @@ fn rewrite_where_clause_rfc_style(
        && 6 + preds_str.len() <= shape.width
    {
        String::from(" ")
+    } else if context.config.where_single_line() {


This looks like it could be ||ed on to the previous if condition

nrc · 2017-10-05T12:38:22Z

src/items.rs

        SeparatorTactic::Never
    } else {
        context.config.trailing_comma()
    };

+    let shape_tactic = if context.config.where_single_line() {


Do we need to check that there is only one item in the where clause here?

nrc · 2017-10-05T12:39:39Z

src/items.rs

+        (_, Density::CompressedIfEmpty, _) if where_clause.predicates.len() == 1 && !has_body => {
+            false
+        }
+        (BraceStyle::SameLineWhere, _, _) if !where_clause.predicates.is_empty() => true,


I think this code would be easier to understand if broken up into an if check before the match.

nrc · 2017-10-05T12:40:59Z

tests/target/configs-where_single_line.rs

+
+
+fn lorem_two_items<Ipsum, Dolor, Sit, Amet>() -> T 
+where Ipsum: Eq, Lorem: Eq {


This should have regular where clause formatting, not single-lined (where_single_line should only apply if there is a single item in the clause).

nrc · 2017-10-05T12:43:05Z

tests/target/configs-where_single_line.rs

+where Ipsum: Eq, Lorem: Eq {
+    // body
+}
+
 fn lorem<Ipsum, Dolor, Sit, Amet>() -> T 


Could you also add a test for a single-item where clause where the rest of the signature covers multiple lines? (That should not get single-lined).

Could help me with this one? We already have one single-item where clause. What do you mean by "rest of the signature"?

If we would put the function signature on multiple lines (e.g., if we use mutliple lines for arguments), then we should always use the multiple line form for the where clause.

afshinm · 2017-10-06T23:44:21Z

Thanks for your feedbacks @nrc. I just refactored some parts of the PR and apologies for that mistake, I thought we want to inline the where clause regardless of the number of items in the where clause.

Let me know what you think!

nrc · 2017-10-07T05:24:48Z

src/items.rs

-        (BraceStyle::SameLineWhere, _) if !where_clause.predicates.is_empty() => true,
+        (_, Density::Compressed) if len == 1 => false,
+        (_, Density::CompressedIfEmpty) if len == 1 && !has_body => false,
+        (BraceStyle::SameLineWhere, _) if !is_empty => true,


Could just use len == 0 and avoid the is_empty variable.

nrc · 2017-10-07T05:25:06Z

src/items.rs

@@ -2282,11 +2282,17 @@ fn compute_budgets_for_args(
 }

 fn newline_for_brace(config: &Config, where_clause: &ast::WhereClause, has_body: bool) -> bool {
+    let len = where_clause.predicates.len();


I would call this predicate_count rather than len

nrc · 2017-10-07T05:25:53Z

src/items.rs

-    let comma_tactic = if where_clause_option.suppress_comma {
-        SeparatorTactic::Never
+    let comma_tactic =
+        if where_clause_option.suppress_comma || context.config.where_single_line() && len == 1 {


Could you factor out context.config.where_single_line() && len == 1 into a variable?

afshinm · 2017-10-07T08:50:28Z

Thanks @nrc, I just refactored those parts.

If we would put the function signature on multiple lines (e.g., if we use mutliple lines for arguments), then we should always use the multiple line form for the where clause.

So, something like:

fn lorem_multi_line<Ipsum, Dolor, Sit, Amet>(
    a: Aaaaaaaaaaaaaaa,
    b: Bbbbbbbbbbbbbbbb,
    c: Ccccccccccccccccc,
    d: Ddddddddddddddddddddddddd,
    e: Eeeeeeeeeeeeeeeeeee,
) -> T where Ipsum: Eq {
    // body
}

must be:

fn lorem_multi_line<Ipsum, Dolor, Sit, Amet>(
    a: Aaaaaaaaaaaaaaa,
    b: Bbbbbbbbbbbbbbbb,
    c: Ccccccccccccccccc,
    d: Ddddddddddddddddddddddddd,
    e: Eeeeeeeeeeeeeeeeeee,
) -> T 
where 
    Ipsum: Eq,
{
    // body
}

Is that correct?

nrc · 2017-10-13T13:42:18Z

Is that correct?

Yes, correct.

afshinm · 2017-10-22T18:27:04Z

@nrc I'm struggling to finish the last part, to skip the where_single_line option when the rest of the signature is multi-lined.

I was thinking about doing a "\n" search and see if we have any new lines in the other parts and then assume that we have to skip (e.g. when there are multiple items in the signature). What do you think about this idea? Is there any better ways to do this?

nrc · 2017-10-26T20:31:36Z

What do you think about this idea? Is there any better ways to do this?

Searching for \n is a fine way to do this - we do it often elsewhere.

afshinm · 2017-10-27T09:53:59Z

@nrc cheers Nick, let me see what can I do this weekend. 🦀

…multilined

afshinm · 2017-11-04T12:25:48Z

@nrc OK, I just pushed the changes you've requested. Let me know what you think.

nrc · 2017-11-05T22:53:09Z

Awesome, thank you!

afshinm · 2017-11-06T08:35:30Z

Thank you Nick!

remram44 · 2018-03-31T22:46:19Z

src/config.rs

@@ -574,6 +574,7 @@ create_config! {
    // function decl?
    // 2. Currently options `Tall` and `Vertical` produce the same output.
    where_density: Density, Density::Vertical, false, "Density of a where clause";
+    where_single_line: bool, false, false, "To force single line where layout";


Both the name of the option (where single line do what?) and the "documentation" (where what is layout?) are extremely confusing. Configurations.md does not help; as a user I have no idea what this does.

[edit: filed #2580]

afshinm added 4 commits September 24, 2017 11:19

feat(where): adding where_single_line option and tests

2ad8951

fix(where): adding more tests

25b6a6b

feat(where): changing the shape of where clause when where_single_lin…

ec6738a

…e is true

feat: newline_for_brace should avoid adding a new line where where_si…

e990a98

…ngle_line is true

nrc reviewed Oct 5, 2017

View reviewed changes

fix(items): where_single_line should bypass the multi-item where clauses

0e9f70e

nrc reviewed Oct 7, 2017

View reviewed changes

fix(items): refactoring and removing redundant variables

c46e38b

afshinm added 2 commits November 4, 2017 12:04

fix(items): where_single_line should not be functional when args are …

f26070a

…multilined

merge with upstream

608fc59

fix(config): fixing conflict with upstream

c663833

nrc merged commit f412c87 into rust-lang:master Nov 5, 2017

afshinm deleted the afshinm-single-line-where branch November 6, 2017 08:35

remram44 reviewed Mar 31, 2018

View reviewed changes

remram44 mentioned this pull request Mar 31, 2018

Improve where_single_line help #2580

Merged



		fn lorem_two_items<Ipsum, Dolor, Sit, Amet>() -> T
		where Ipsum: Eq, Lorem: Eq {

Adding where_single_line option #2030

Adding where_single_line option #2030

Uh oh!

Conversation

afshinm commented Oct 4, 2017

Uh oh!

nrc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

afshinm commented Oct 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

afshinm commented Oct 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nrc commented Oct 13, 2017

Uh oh!

afshinm commented Oct 22, 2017

Uh oh!

nrc commented Oct 26, 2017

Uh oh!

afshinm commented Oct 27, 2017

Uh oh!

afshinm commented Nov 4, 2017

Uh oh!

nrc commented Nov 5, 2017

Uh oh!

afshinm commented Nov 6, 2017

Uh oh!

remram44 Mar 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

afshinm commented Oct 7, 2017 •

edited

Loading

remram44 Mar 31, 2018 •

edited

Loading