Skip to content

Use vertical layout for complex attributes #2557

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

Merged
merged 3 commits into from
Mar 28, 2018

Conversation

topecongiro
Copy link
Contributor

Currently rustfmt forces mixed layout when formatting attributes. This is suboptimal when the attribute has complex inner items. This PR fixes it by disallowing mixed layout when every item in the attribute is either list or assignment.

html_favicon_url = "https://doc.rust-lang.org/favicon.ico",
html_root_url = "https://doc.rust-lang.org/nightly/",
html_playground_url = "https://play.rust-lang.org/",
test(attr(deny(warnings))))]
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the PR! I think that we should use block formatting rather than visual formatting though, e.g.,

#![doc(
    html_logo_url = "https://www.rust-lang.org/logos/rust-logo-128x128-blk-v2.png",
    html_favicon_url = "https://doc.rust-lang.org/favicon.ico",
    html_root_url = "https://doc.rust-lang.org/nightly/",
    html_playground_url = "https://play.rust-lang.org/",
    test(attr(deny(warnings))),
)]

(Although I think we need to follow macros and not insert a trailing comma if there is not one there already, since in the future it could be significant).

I think requiring all items in the list to be complex is probably to weak - e.g., imagine an attribute like the above example but with a trailing 0 - we'd still want to multi-line it, IMO. We could either have an any condition, rather than all or a 'more than half' condition (or something like that) or we could have something based on the overall length of the attribute - like always use vertical formatting if it goes multi-line.

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 your review. Actually the current implementation does the opposite of what I said at the top - it uses mixed layout only when all the items are 'simple'. My appologies for wrong description.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have updated the PR to use indent_style when formatting attributes, so that we use block indent style by deafult.

@nrc nrc merged commit 1644b17 into rust-lang:master Mar 28, 2018
@nrc
Copy link
Member

nrc commented Mar 28, 2018

Thanks!

@topecongiro topecongiro deleted the vertical-layout-complex-attrs branch July 21, 2019 05:03
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