WIP: Format expression chains #216
Conversation
|
Updated. I am now relatively content with the output. Still needs tests, a big refactoring and a rebase. I believe this also addressed https://github.com/nrc/rustfmt/issues/194 and https://github.com/nrc/rustfmt/issues/195 to some extent. |
|
Rebased, cleaned up and tested! |
|
Could you squash some of the commits please? |
| bbbbbbbbbbbbbbbbbbb.ccccccccccccccccccccccccccccccccccccc.ddddddddddddddddddddddddddd(); | ||
|
|
||
| bbbbbbbbbbbbbbbbbbb.ccccccccccccccccccccccccccccccccccccc | ||
| .ddddddddddddddddddddddddddd |
There was a problem hiding this comment.
We should try to line up the dots if there is space, i.e.,
+ bbbbbbbbbbbbbbbbbbb.ccccccccccccccccccccccccccccccccccccc
+ .ddddddddddddddddddddddddddd
|
Updated and rebased once again. |
| let mut def_config = String::new(); | ||
| def_config_file.read_to_string(&mut def_config).ok().expect("Couldn't read config."); | ||
| def_config_file.read_to_string(&mut def_config).ok().expect("Couldn\'t read config."); |
There was a problem hiding this comment.
Agreed, but it's not a direct result of this PR. The string is being reformatted where it wasn't earlier.
There was a problem hiding this comment.
Yup. I believe https://github.com/nrc/rustfmt/pull/248 will fix this.
|
r? @cassiersg @nrc |
| }) | ||
| regex.captures_iter(&line) | ||
| .next() | ||
| .map(|capture| { |
There was a problem hiding this comment.
THis seems like an unnecessary link breaking.
There was a problem hiding this comment.
There's quite a few places where do this pattern of line breaking. I do think we should fix it before landing this PR.
There was a problem hiding this comment.
Hmmm.. it's similar to what we do for function arguments, is it not? There we do not let the last argument spill over either.
I agree that it's not the most dense formatting, but I do think it adds clarity.
There was a problem hiding this comment.
Perhaps it is personal prejudice, but some how a chain of fields/methods feels more unitary and less like a splitable list than function arguments. To me it feels like somewhere where we should only line-break where we have to. I do agree that it often adds clarity, but I think those cases are exceptional enough that having a heuristic to take into account the original formatting would be optimal.
I'd be happy to have an option with different heuristic levels - avoid line breaks, eagerly break, and avoid unless already multi-line.
|
This is a cool PR - it means we format a lot of code we didn't before and we can do a better job too. I'd like to get the tweeks to the heuristics in before landing though - I can imagine that this could cause some big changes to users, so I want to get it as close to perfect as possible first go. |
|
Yea, sounds good. Thanks for the review. |
|
This has been updated and (monster) rebased. All comments have been addressed, except for the one about non breaking chains after function calls. I've tried this, but it generally gives pretty poor results. Some examples: + let line_regex = regex::Regex::new(r"(^\s*$)|(^\s*//\s*rustfmt-[^:]+:\s*\S+)")⏎
+ .ok()⏎
+ .expect("Failed creating pattern 2.");⏎
- let line_regex =⏎
- regex::Regex::new(r"(^\s*$)|(^\s*//\s*rustfmt-[^:]+:\s*\S+)").ok()⏎
- .expect("Failed creating \⏎
- pattern 2.");⏎+ CharClasses::new(text.chars())⏎
+ .filter_map(|(s, c)| {⏎
+ match s {⏎
+ CodeCharKind::Normal => Some(c),⏎
+ CodeCharKind::Comment => None,⏎
+ }⏎
+ })⏎
+ .collect()⏎
- CharClasses::new(text.chars()).filter_map(|(s, c)| {⏎
- match s {⏎
- CodeCharKind::Normal => Some(c),⏎
- CodeCharKind::Comment => None,⏎
- }⏎
- })⏎
- .collect()⏎ |
|
Which is before and which after? |
|
The fact that you have to ask shows that you don't feel as strongly about those as I do! :p |
| // Test chain formatting. | ||
|
|
||
| fn main() { | ||
| // Don't put chains on a single list if it wasn't so in source. |
|
Awesome, thanks for the changes! r+ with the few nits cleared up. |
Don't make a single line chain when it is was multi line in the source; allow overflow of the last chain element onto the next lines without breaking the chain.
WIP: Format expression chains
This PR tries to deal with formatting expression chains. It is still very much a work in progress!
The basic idea is to not look at one point in the expression tree, but look at chains of certain operations, those separated by dots. When all these expression parts fit on a single line, we format them as such, and otherwise put each on their own line. This is not unlike the formatting of function arguments.
I think this generally looks fairly decent, but they are some ugly corner cases. I'd love to receive input on how we could deal with those, as well as the general case.