Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[Tests] Nested formatting issues #8224

Merged
merged 1 commit into from
Feb 6, 2023

Conversation

LunicLynx
Copy link
Contributor

@LunicLynx LunicLynx commented Feb 4, 2023

Here are some Tests demonstrating razor formatting issues.

This PR contains one two change sets.

  • The first has all the Tests that fail.
  • The second fixes one of these test.

I tried to fix the other two as well but wasn't able to. Also I'm not sure if my approach in fixing the first one is aligned to how things should be done.

Please check if my assumption about supposed formatting is actually correct.

If you can give me some guidance I'm willing to fix the other two tests as well.

Issues tracked in:
#8227
#8228
#8229

@LunicLynx LunicLynx requested a review from a team as a code owner February 4, 2023 11:57
// MarkupTagHelperAttributeSyntax { TagHelperAttributeInfo: { Bound: true } } or
var isInHtmlAttributeName = owner.AncestorsAndSelf().Any(
n => n is MarkupTagHelperAttributeSyntax { TagHelperAttributeInfo: { Bound: true } } markupTagHelperAttribute
? dontFormatIfAttributeIsInNewLine || markupTagHelperAttribute.NamePrefix.LiteralTokens[0].Kind != SyntaxKind.NewLine
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The important change.

@LunicLynx LunicLynx changed the title Nested formatting issues [Tests] Nested formatting issues Feb 5, 2023
Copy link
Member

@davidwengier davidwengier 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 contribution, and thanks for the failing tests. If nothing else, committing those with a Skip attribute, and creating an issue, would be an awesome contribution.

Having said that, the fix here doesn't feel right to me. ShouldFormat in this case is basically being asked the question "I found this piece of C# code, should I format it according to C# rules?". The method IsInHtmlAttributeName was added because attributes can now appear to be C# for the purposes of Find All Refs, Rename, etc. but we don't want them to be seen as C# for the purposes of formatting. I think that statement holds true, even if the attribute is the first thing on the line.

I think the fix here would be somewhere in the AdjustIndentationAsync method, or perhaps the logic for StartsInCSharpContext/StartsInHtmlContext. Essentially, ShouldFormat was fixed, but something else must be still acting as though the attribute is C#.

One suggestion to dig in further, if you remove the Caption property from the component definition temporarily, those attributes will no longer map to any C#, and hopefully that will highlight where the code is doing different things.

// MarkupTagHelperAttributeSyntax { TagHelperAttributeInfo: { Bound: true } } or
var isInHtmlAttributeName = owner.AncestorsAndSelf().Any(
n => n is MarkupTagHelperAttributeSyntax { TagHelperAttributeInfo: { Bound: true } } markupTagHelperAttribute
? dontFormatIfAttributeIsInNewLine || markupTagHelperAttribute.NamePrefix.LiteralTokens[0].Kind != SyntaxKind.NewLine
Copy link
Member

Choose a reason for hiding this comment

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

markupTagHelperAttribute.NamePrefix.LiteralTokens[0].Kind != SyntaxKind.NewLine

If this logic does end up staying, I'm not sure using the syntax tree for this is the best. Should be able to get the first non-whitespace character of the line with the following code:

sourceText.GetLineAndOffset(absoluteIndex, out var lineIndex, out var offset);
var line = sourceText.Lines[lineIndex];
var lineStart = line.Start + line.GetFirstNonWhitespaceOffset();

Comparing that lineStart to the start of the actual attribute name seems to me like it would be better, in case the tree structure changes. eg, what if there is whitespace before the newline, is that now part of the NamePrefix too?

@LunicLynx
Copy link
Contributor Author

Thanks for the feedback I will investigate. And submit another PR for tests only.

@LunicLynx LunicLynx force-pushed the nested-formatting-issues branch from 6c668db to 774eb52 Compare February 6, 2023 08:15
@LunicLynx
Copy link
Contributor Author

Removed the fix change set from this PR.
Also created issues for each test.

#8227
#8228
#8229

@LunicLynx LunicLynx force-pushed the nested-formatting-issues branch from 774eb52 to e44b640 Compare February 6, 2023 08:53
Copy link
Member

@davidwengier davidwengier left a comment

Choose a reason for hiding this comment

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

Thanks very much for the contribution :)

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants