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

FormatWriter: fix alignment of multiline defns #2753

Merged
merged 4 commits into from
Sep 20, 2021

Conversation

kitbellew
Copy link
Collaborator

Fixes #2750.

@@ -918,15 +918,15 @@ object a {
}
>>>
object a {
def foo: Int = {
def foo: Int = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we have additional align here and in the two examples below?

If I understand the intention, we should not align if the statements are not just below one another. That's what seems to be happening in #2750 4 blocks, no blanks test case.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

one of things highlighted in the submitted issue is the inconsistent handling of multiline definitions with and without curly braces (blocks vs no blocks). so that was one of the fixes.

both here and below, align.preset = most is used and it enables multiline alignment. since these methods are not separated by blank lines (those should interrupt alignment), they are aligned.

the difference with the case below is that includes alignment of : (and they do not align, hence = we don't get to =), while here there are only equals.

Copy link
Contributor

Choose a reason for hiding this comment

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

and they do not align

Is it an issue that there is multiple : on the line? We do not try to align in that case?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the way I've implemented it now is that parameter colons should only align with parameters from the same definition (that is, the align owner is the definition) whereas the align owner for the equals is the template which contains the definition.

you can only have one align owner for all tokens on a given line, so it's either colons or equals but not both.

might need to adjust the logic to figure out which should be used when both are present. give me some time, please.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ptal, added a commit to choose the largest align container.

Copy link
Contributor

@tgodzik tgodzik left a comment

Choose a reason for hiding this comment

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

LGTM!

@kitbellew kitbellew merged commit 0a5272d into scalameta:master Sep 20, 2021
@kitbellew kitbellew deleted the 2750 branch September 20, 2021 15:38
Copy link

@mwachs5 mwachs5 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 fix!

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.

Blank Newlines between defs aren't enough to break alignment (3.0.0)
3 participants