Refactor chain formatting and fix some bugs #2838
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
r? @topecongiro |
|
@topecongiro ping - will you get a chance to review this PR soon? If not, I'll merge and we can revisit later if necessary. |
topecongiro
approved these changes
Jul 23, 2018
There was a problem hiding this comment.
My apologies for the late review, I missed the first notification email. The code looks good to me. My only concern is the unindentation of the chain element that it is not orphaned, though if it is hard to fix right away, we can revisit this after merging this.
| .map(|l| l.unwrap()) | ||
| .enumerate(); | ||
| .map(|l| l.unwrap()) | ||
| .enumerate(); |
There was a problem hiding this comment.
I am not sure whether we should unindent when chain elements does not become orphaned.
src/chains.rs
Outdated
| // foo.bar | ||
| // .baz() | ||
| // ``` | ||
| // If `bar` were not part of the root, then baz would be orphaned and 'float'. |
There was a problem hiding this comment.
Bikeshedding: in this case, the element that gets orphaned is foo, not baz:
foo
.bar
.baz()
|
Also we need a rebase. |
* More often combine a chain item to the previous line (if it is a block) * Don't indent if the parent is a block This is not perfect and doesn't pass tests, but I need to refactor to make more progress
Just refactoring, lots of code dup here, but it should get better...
And create `Chain` and `ChainItem` structs
pre-process the expression tree to get a list of chain items.
* remove unnecessary clone * comment formatting * fix bug with `?` collection * respect the heuristic if the root is more than just the parent
…ame line would introduce an open block or similar
This problem came to light due to the chains changes, but effects other code too. It only happens rarely, e.g.,
before this fix:
```
match foo {
MacroArgKind::Delimited(ref delim_tok, ref args) => rewrite_delimited_inner(
delim_tok,
args,
).map(|(lhs, inner, rhs)| format!("{}{}{}", lhs, inner, rhs)),
};
```
after:
```
match foo {
MacroArgKind::Delimited(ref delim_tok, ref args) => {
rewrite_delimited_inner(delim_tok, args)
.map(|(lhs, inner, rhs)| format!("{}{}{}", lhs, inner, rhs))
}
}
```
Closes rust-lang#2773 Closes rust-lang#2786
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There's quite a lot of changes to formatting here. Most are net positive or neutral (IMO). There are a few changes to visual formatting that are not idea, but since that is not the default I think it is worth the price.