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

use the lists module for formatting the generic bounds #3680

Closed
wants to merge 11 commits into from

Conversation

scampi
Copy link
Contributor

@scampi scampi commented Jul 12, 2019

  • Fix how to extract post comments in a list with separators at the front. Fix extraction of a comment between the type bounds and the opening curly bracket.
  • Support explicit padding passed through the separator when writing a list.

Close #3669
Close #2055

@scampi
Copy link
Contributor Author

scampi commented Jul 12, 2019

Is the whitespace after + with type_punctuation_density = Compressed intended, i.e., https://github.com/rust-lang/rustfmt/pull/3680/files#diff-dfd26227c4842bd3288e9b1f53f0089fL844 ?

Given it's Compressed, in the current state of the PR the whitespace is removed, i.e.,
https://github.com/rust-lang/rustfmt/pull/3680/files#diff-9015fa417c17e7ba1e7d928f6daef412R1

@scampi
Copy link
Contributor Author

scampi commented Jul 16, 2019

About the comment above, below is the diff against master of tests/target/types_compressed.rs: there was a whitespace added after + at the beginning of each line.

10,47c10,47
< +         + Foo
< +         + Foo
< +         + Foo
< +         + Foo
< +         + Foo
< +         + Foo
< +         + Foo
< +         + Foo
< +         + Foo
< +         + Foo
< +         + Foo
< +         + Foo
< +         + Foo
< +         + Foo
< +         + Foo
< +         + Foo
< +         + Foo
< +         + Foo
< +         + Foo
< +         + Foo
< +         + Foo
< +         + Foo
< +         + Foo
< +         + Foo
< +         + Foo
< +         + Foo
< +         + Foo
< +         + Foo
< +         + Foo
< +         + Foo
< +         + Foo
< +         + Foo
< +         + Foo
< +         + Foo
< +         + Foo
< +         + Foo
< +         + Foo
< +         + 'a+'a+'a+'a+'a {
---
> +         +Foo
> +         +Foo
> +         +Foo
> +         +Foo
> +         +Foo
> +         +Foo
> +         +Foo
> +         +Foo
> +         +Foo
> +         +Foo
> +         +Foo
> +         +Foo
> +         +Foo
> +         +Foo
> +         +Foo
> +         +Foo
> +         +Foo
> +         +Foo
> +         +Foo
> +         +Foo
> +         +Foo
> +         +Foo
> +         +Foo
> +         +Foo
> +         +Foo
> +         +Foo
> +         +Foo
> +         +Foo
> +         +Foo
> +         +Foo
> +         +Foo
> +         +Foo
> +         +Foo
> +         +Foo
> +         +Foo
> +         +Foo
> +         +Foo
> +         +'a+'a+'a+'a+'a {

@scampi scampi marked this pull request as ready for review July 16, 2019 23:54
@scampi
Copy link
Contributor Author

scampi commented Jul 23, 2019

@topecongiro what do you think of the proposed changes ?

Copy link
Contributor

@topecongiro topecongiro left a comment

Choose a reason for hiding this comment

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

My apologies for the late review. The new format when type_punctuation_density is set to Compressed seems good to me.

tests/target/issue-3669.rs Outdated Show resolved Hide resolved
@scampi scampi force-pushed the format-bounds-list branch from e168a45 to 54da77a Compare July 24, 2019 22:51
+ Serialize // comment8
+ for<'a> Deserialize<'a> // comment9
{
type DoubleState: Copy // Note(Evrey): Because Rust is drunk. 1
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like the first comment is not aligned with others. Is this intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the alignement in 3850fb1#diff-11ffc82e7dc3c14ae179a08592fd9c26R28 got fixed but not this one yet, i'll try to see if I can.

It seems that the shape offset is not correctly set: the extra space taken by the type name is not considered by the first item.

@scampi
Copy link
Contributor Author

scampi commented Sep 25, 2019

@topecongiro sorry for the delay, I have now gated the changes proposed in this PR. Could you give this another look? Thanks!

pub fn do_something<'a, T: Trait1+Trait2+'a>(
&fooo: u32,
) -> impl Foo
+Foo
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we still want to indent these +Foos even when type_punctuation_density is set to Compressed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indenting these +Foo would go against the fix for #3701: https://github.com/rust-lang/rustfmt/pull/3731/files#diff-dfd26227c4842bd3288e9b1f53f0089fR751

join_bounds(context, shape, it, false)

The false argument instructs join_bounds not to indent; to indent +Foo I would have to pass true instead.

BTW This non-indentation happens regardless of type_punctuation_density.

What should I do here ?

@topecongiro
Copy link
Contributor

Closing this due to inactivity; if you have any follow-up, please feel free to reopen this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants