Skip to content

Add reformating for parentheses #74

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 4 commits into from
May 24, 2015
Merged

Conversation

cassiersg
Copy link
Contributor

I'm quite new to Rust, so I picked this for practice. Please tell me if it's not idiomatic for Rust.

debug!("rewrite_paren, subexpr_str: `{}`", subexpr_str);
let mut lines = subexpr_str.rsplit('\n');
let last_line_len = lines.next().unwrap().len();
let last_line_offset = if lines.next().is_none() {offset+1} else {0};
Copy link
Member

Choose a reason for hiding this comment

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

better to spread this if expression over multiple lines

Copy link
Member

Choose a reason for hiding this comment

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

and actually better to use a match expression than if ...is_none()

@nrc
Copy link
Member

nrc commented May 24, 2015

Thanks for the PR! From an idiomatic Rust point of view this looks great, the only non-idomatic thing I noticed was using if ...is_none()... rather than match ....

I think I would format so there is never a dangling ). I.e., you'd always have (a + b + c) or

(a + b +
 c + d)

never

(a + b + c
 )

If we ever need that one char to fit in the width, it would be better to either modify the sub-expression or to modify the enclosing expression.

@cassiersg
Copy link
Contributor Author

This last commit avoid dangling ), by modifying the sub-expression.

@cassiersg
Copy link
Contributor Author

Thank you for the prompt review.

@nrc
Copy link
Member

nrc commented May 24, 2015

Sweet, thanks

nrc added a commit that referenced this pull request May 24, 2015
Add reformating for parentheses
@nrc nrc merged commit c385688 into rust-lang:master May 24, 2015
@cassiersg cassiersg deleted the paren-expr branch May 25, 2015 10:17
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