Skip to content

Conversation

@davidwengier
Copy link
Member

Hopefully fixes #4472, though I'm not sure I know how to test that theory 😁

@davidwengier davidwengier requested review from a team as code owners October 24, 2022 10:25
@@ -1,19 +1,19 @@
{
"$schema": "https://json.schemastore.org/component-detection-manifest.json",
Copy link
Member Author

Choose a reason for hiding this comment

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

The whitespace changes here are annoying, but PowerShell doesn't let you control whitespace, and at least once this change is made, future runs of the tool will have minimal changes.

Copy link
Contributor

@ryanbrandenburg ryanbrandenburg left a comment

Choose a reason for hiding this comment

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

Looks like this "broke" the Grammar tests (hey, that means they're running!) but they aren't showing up in the tests tab of AzDO. I THINK you can fix that by un-commenting https://github.com/dotnet/razor-tooling/blob/main/azure-pipelines.yml#L281-L288.

@davidwengier

This comment was marked as outdated.

@azure-pipelines

This comment was marked as outdated.

@davidwengier
Copy link
Member Author

oh.. its a whitespace issue, isn't it? 🤦‍♂️

"patterns": [
{
"include": "source.cs"
"include": "source.cs#class-or-struct-members"
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the fix for the nullable issue, specifically telling TextMate that the contents of @code and @functions blocks are class members.

At least thats my intepretation of what this does :)

Copy link
Member

Choose a reason for hiding this comment

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

That sounds plausible. The C# TextMate grammar allows for top-level statements, and I wonder if that's tripping us up.

@davidwengier
Copy link
Member Author

Please re-review @ryanbrandenburg, and would love your review @DustinCampbell because I assume you both know more about this than me. As annoying as it is, reviewing the test snapshot file would be very helpful :)

@davidwengier davidwengier requested review from DustinCampbell and allisonchou and removed request for a team October 25, 2022 01:26
@davidwengier

This comment was marked as outdated.

@azure-pipelines

This comment was marked as outdated.

Copy link
Member

@DustinCampbell DustinCampbell left a comment

Choose a reason for hiding this comment

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

This seems reasonable, but I'm not sure why CI is busted.

"patterns": [
{
"include": "source.cs"
"include": "source.cs#class-or-struct-members"
Copy link
Member

Choose a reason for hiding this comment

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

That sounds plausible. The C# TextMate grammar allows for top-level statements, and I wonder if that's tripping us up.

"version": "https://github.com/microsoft/vscode/tree/ecb3421521db4764becfd198a79ca88b6aad73c2"
}
],
"version": 1
Copy link
Member

Choose a reason for hiding this comment

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

Did you mean to change the formatting here so dramatically?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes and no. No, in that Powershell wrote out the file like this, and I couldn't work out how to get it to stop, but yes, in that I don't think its a problem and at least future updates will be minimal.

Copy link
Contributor

Choose a reason for hiding this comment

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

if it's ConvertTo-Json in powershell, I believe -Compress removes indentation/whitespace

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, but that makes it all one line, and even uglier IMO 😛

Copy link
Contributor

Choose a reason for hiding this comment

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

ah, I haven't used it in a while. I tried 🤷

113,1,HTML Tag Delimiter,
124,1,HTML Tag Delimiter,
125,7,RazorComponentElement,
132,1,HTML Tag Delimiter,
Copy link
Member Author

Choose a reason for hiding this comment

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

Just when you think you might understand classification, along comes this change, which is apparently caused by the changes in this PR, but I am definitely not smart enought to work out why!

Copy link
Member

Choose a reason for hiding this comment

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

Did you get to the bottom of this? That's definitely a problem.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not entirely.. the difference is that the whitespace before the /> is not classified as a tag delimeter any more, which seems completely fine to me, but I can't directly explain why it changed now. The html textmate files in this test project were not up to date with our real ones though 🤷‍♂️

Copy link
Member

Choose a reason for hiding this comment

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

Oh! OK, it's just the whitespace. That's totally fine. I thought it was the />.

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.

Razor TextMate grammar does not handle nullability well in @code blocks

4 participants