-
Notifications
You must be signed in to change notification settings - Fork 229
Fix formatting of lines that start with component attributes #8279
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
Fix formatting of lines that start with component attributes #8279
Conversation
| var lineStart = line.GetFirstNonWhitespacePosition() ?? line.Start; | ||
| var lineStartSpan = new TextSpan(lineStart, 0); | ||
| if (!ShouldFormat(context, lineStartSpan, allowImplicitStatements: true)) | ||
| if (!ShouldFormatLine(context, lineStartSpan, allowImplicitStatements: true)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This ShouldFormat method is arguably still doing too much, but this particular usage has annoyed me before, so I done fixed it.
| } | ||
|
|
||
| bool IsInHtmlAttributeName() | ||
| bool IsInBoundComponentAttributeName() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method name was doing a poor job of describing what it did. Also expanded the comment below to be clearer.
| public override void VisitMarkupTagHelperAttribute(MarkupTagHelperAttributeSyntax node) | ||
| { | ||
| Visit(node.Value); | ||
| WriteBlock(node, FormattingBlockKind.Tag, n => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This updates tag helper attributes to match the logic of html tag attributes, in the method above.
I don't fully understand:
- What this is doing
- Why it wasn't doing it before
- Why only one of the tests was fixed with this change
- Why none of the existing tests broke with this change
Formatting! :)
| @if (true) | ||
| { | ||
| <a_really_long_tag_name Id="comp1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added this because html tags formatted correctly, and component tags didn't, so it was convenient when debugging to see what was happening that was different. Figured I'd leave it 🤷♂️
|
Awesome. I tried myself on this, but was not getting anywhere, only more confused. Great that you made progress on this! |
|
@dotnet/razor-tooling would appreciate a review if you have time :) |
To celebrate finally being able to build on my machine, I took a break from firefighting to write some code.
Fixes #8227
Fixes #8229
I cannot explain why this doesn't fix the third skipped test, but I wanted to timebox the work so I did.
Thanks @LunicLynx for creating the tests for these, made it much easier