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

Split long fields in structs #1424

Merged
merged 1 commit into from
Mar 30, 2017
Merged

Split long fields in structs #1424

merged 1 commit into from
Mar 30, 2017

Conversation

topecongiro
Copy link
Contributor

Closes #1412.

Copy link
Member

@nrc nrc left a comment

Choose a reason for hiding this comment

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

Thank you for the PR, this is a great fix! There are a few minor things to fix in the inline comments.

src/items.rs Outdated
.rewrite(context,
Shape::legacy(budget, shape.indent + last_line_width));
match type_rewrite {
Some(ty) => {
Copy link
Member

Choose a reason for hiding this comment

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

we should check here to see if there are linebreaks in ty, if so, then we should take the other branch (because we prefer to split at the : rather than in the type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nrc Thank you for you feedbacks!
I have a question here, is it better to format in the following way?
In the first struct its field does not exceed 100 letters, but contains linebreaks.

struct Deep {
    deeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeep: node::Handle<IdRef<'id, Node<K, V>>,
                                                                        Type,
                                                                        NodeType>

to

struct Deep {
    deeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeep:
        node::Handle<IdRef<'id, Node<K, V>>, Type, NodeType>,
}

Copy link
Member

Choose a reason for hiding this comment

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

the second version is better IMO

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks!

src/items.rs Outdated
type_offset.to_string(&context.config),
s)
}));
Some(result + &type_rewrite)
Copy link
Member

Choose a reason for hiding this comment

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

I think this code could be simpler, rather than map the rewrite, you could make the returned value Some(format!({}\n{}{}), result, type_offset.to_string(...), type_rewrite)

the_quick_brown_fox_jumps_over_the_lazy_dog:
AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA,
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a test where the type is split across multiple lines please?


mod long_struct {
type A = i32;
type AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA = i32;
Copy link
Member

Choose a reason for hiding this comment

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

You don't need these - rustfmt tests don't need to compile, only parse

@topecongiro
Copy link
Contributor Author

Rebased.

src/items.rs Outdated
.rewrite(context,
Shape::legacy(budget, shape.indent + last_line_width)) {
Some(ty) => {
if ty.contains('\n') {
Copy link
Member

Choose a reason for hiding this comment

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

Could you guard the match arm with this, rather have an if block?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Certainly.

This commit splits long fields in structs.
Closes rust-lang#1412.
@nrc nrc merged commit 4f226eb into rust-lang:master Mar 30, 2017
@nrc
Copy link
Member

nrc commented Mar 30, 2017

Thank you!

@topecongiro topecongiro deleted the long-struct branch June 16, 2017 00:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants