Support ; and } format on type in mixed scenarios #2585
Conversation
ajaybhargavb
requested review from
NTaylorMullen,
ryanbrandenburg and
TanayParikh
as code owners
ajaybhargavb
force-pushed
the
ajbaaska/better-ontype-formatting
branch
from
0085b37
to
8fbea5b
Compare
| var offset = (int)offsetProperty.GetValue(result); | ||
|
|
||
| var resultLine = changedText.Lines.GetLinePosition(basePosition); | ||
| var indentation = resultLine.Character + offset; | ||
|
|
||
| // IIndentationService always returns offset as the number of spaces. | ||
| // So if the client uses tabs instead of spaces, we need to convert accordingly. | ||
| if (!context.Options.InsertSpaces) |
There was a problem hiding this comment.
Why don't we still need to do this?
There was a problem hiding this comment.
I changed the place where I consumed this to operate on offset instead of levels. I needed to do that because the HTML formatter doesn't always format in exact multiples of levels.
Example,
<div class=foo
id=fooid>
</div>
| } | ||
|
|
||
| private static bool ShouldCleanup(FormattingContext context, Position position) | ||
| protected bool ShouldFormat(FormattingContext context, Position position, bool allowImplicitStatements) |
There was a problem hiding this comment.
implicit statement or implicit expression? Also, is the reason why the flag is even here is because of the differences between OnTypeFormatting and regular Formatting in regards to implicit expressions?
There was a problem hiding this comment.
implicit statement or implicit expression?
Statements like @if (true){...}.
is the reason why the flag is even here is because of the differences between OnTypeFormatting and regular Formatting
That too. But this is mainly the difference between whether to format using the C# formatter and whether to cleanup that source mapping. They both use ShouldFormat to decide whether they should operate on it. The job of CleanupDocument is to introduce a newline at the start and end of each source mapping. But we can't do that for implicit statements because then it would be wrong like below,
@
if(true){...}
Note: Implicit expressions are never formatted or cleaned up. So didn't need to provide a parameter for that.
src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Formatting/FormattingPassBase.cs
Show resolved
Hide resolved
|
|
||
| private void CleanupSourceMappingEnd(FormattingContext context, Range sourceMappingRange, List<TextChange> changes) |
There was a problem hiding this comment.
Wonder if "Cleanup" is the right verbiage. This method is all about separating language contexts. Also, it adds to the changes collection
There was a problem hiding this comment.
I agree but all the other names I can think of are too long. I'll try to think of a better name.
There was a problem hiding this comment.
I tried really hard but couldn't come up with a concise name :(
|
|
||
| namespace Microsoft.AspNetCore.Razor.LanguageServer.Formatting | ||
| { | ||
| internal class OnTypeFormattingStructureValidationPass : FormattingPassBase |
There was a problem hiding this comment.
So this was just restricting where the ontype formatting would occur. The pre-existing structure validation pass that validates that no content has been "destroyed" still exists right?
There was a problem hiding this comment.
pre-existing structure validation
The other validation we currently have are,
- Content validation (makes sure only whitespace is nuked)
- Diagnostic validation (makes sure we don't introduce new diagnostics. This works surprisingly well in most cases)
| @@ -41,5 +41,23 @@ public static int CompareTo(this Position position, Position other) | |||
| var result = position.Line.CompareTo(other.Line); | |||
| return (result != 0) ? result : position.Character.CompareTo(other.Character); | |||
| } | |||
|
|
|||
| public static bool IsValid(this Position position, SourceText sourceText) | |||
|
💯 looks like there are some test failures though. |
Looks like some tests became flaky. Nothing here looks like it can introduce flakiness. I wonder how 🤔 |
* Fix Formatting Flakyness * Serialize InitializeFile Based Tests
Fixes:
Summary of the changes
CleanupDocumentto cleanup both the start and end of the source mappings (we used to only do start)Pre-LSP:
Now:
