New baselines for RazorDirectivesTest #2621

ajaybhargavb · 2018-09-27T23:45:33Z

Another file with lots of tests

ajaybhargavb · 2018-09-27T23:47:58Z

...rDirectivesTest/BuiltInDirectiveDoesNotErorrIfNotAtStartOfLineBecauseOfWhitespace.cspans.txt

@@ -1,7 +1,7 @@
 Markup span at (0:0,0 [2] ) (Accepts:Any) - Parent: Markup block at (0:0,0 [26] )
-Code span at (2:1,0 [2] ) (Accepts:Any) - Parent: Directive block at (2:1,0 [24] )


Can I fight for this? I don't get why the whitespace prior to a directive needs to be part of the directive. The way I designed it, the directive starts with a transition@.

I'll let @NTaylorMullen weigh in on this.

How do you plan to implement trivia for directives? In my mind the leading whitespace of a directive should have the following characteristics:

Cannot contain any content

Should have 0 intellisense/completion at design time

Can be 0+ whitespace characters long.

With whatever trivia approach you have in mind would it make the above characteristics easy to implement/understand?

From what I understand from Roslyn, "trivia" is generally associated with tokens rather than specific nodes. Leading trivia of any node is basically the leading trivia of its first token. https://github.com/dotnet/roslyn/blob/master/src/Compilers/Core/Portable/Syntax/SyntaxNode.cs#L911.
It does make sense to have the leading whitespace as the leading trivia for the transition @. But don't we already allow comments before the start of a directive? Not sure if it makes sense to have that as part of trivia. Having said that, I haven't given a lot of thought to how we want to do trivia yet. So, this might need more discussion.

But don't we already allow comments before the start of a directive?

Nah

Do you think it makes sense to design (not implement) the plans for trivia now so we can make better choices with the SyntaxTree? Or does it make sense to wait and do reactive changes later?

Spoke to @NTaylorMullen offline. We are not going to make this consistent with the old world because the old world already has different behavior depending on the contents of the previous line. I plan to keep this change and test this in the editor to make sure we didn't break anything.

ajaybhargavb · 2018-09-27T23:48:23Z

...rectivesTest/ExtensibleDirectiveDoesNotErorrIfNotAtStartOfLineBecauseOfWhitespace.cspans.txt

@@ -1,7 +1,7 @@
 Markup span at (0:0,0 [2] ) (Accepts:Any) - Parent: Markup block at (0:0,0 [46] )
-Code span at (2:1,0 [2] ) (Accepts:Any) - Parent: Directive block at (2:1,0 [44] )


rynowak · 2018-09-28T02:29:47Z

...azorDirectivesTest/DirectiveDescriptor_FileScopedSinglyOccurring_ErrorsIfDuplicate.stree.txt

+                            Identifier;[Encoding];
+                            Dot;[.];
+                            Identifier;[ASCIIEncoding];
+                        MarkupEphemeralTextLiteral - [42..44)::2 - [LF] - Gen<None> - SpanEditHandler;Accepts:Whitespace


I see this and I lol? What is this?

Naming is hard! I had originally named it MarkupHiddenTextLiteral. This is basically content that is necessary for design time but we don't want them rendered in output.

It seems like I have a lot to learn

NTaylorMullen · 2018-11-02T21:26:15Z

.../ParserTests/RazorDirectivesTest/AddTagHelperDirective_InvalidLookupText_AddsError.stree.txt

+                CSharpTransition - [0..1)::1 - Gen<None> - SpanEditHandler;Accepts:None
+                    Transition;[@];
+                RazorDirectiveBody - [1..17)::16
+                    RazorMetaCode - [1..13)::12 - Gen<None> - SpanEditHandler;Accepts:None


Might eventually make sense to lift this outside of the directive body since it represents the directive identifier.

Added to https://github.com/aspnet/Razor/issues/2619#issue-364659549

NTaylorMullen · 2018-11-02T21:28:55Z

.../TestFiles/ParserTests/RazorDirectivesTest/DirectiveDescriptor_AllowsNullableTypes.stree.txt

+                        CSharpStatementLiteral - [8..15)::7 - [string?] - Gen<DirectiveToken {;Type;Opt:False}> - DirectiveTokenEditHandler;Accepts:NonWhitespace
+                            Keyword;[string];
+                            QuestionMark;[?];
+                        CSharpStatementLiteral - [15..16)::1 - [ ] - Gen<None> - SpanEditHandler;Accepts:Whitespace


Ya we'll also eventually want to change these whitespace markers to be a different type of significant

Added to https://github.com/aspnet/Razor/issues/2619#issue-364659549

NTaylorMullen · 2018-11-02T21:30:37Z

...arserTests/RazorDirectivesTest/DirectiveDescriptor_NoErrorsSemicolonAfterDirective.stree.txt

+    MarkupBlock - [0..19)::19
+        MarkupTextLiteral - [0..0)::0 - [] - Gen<Markup> - SpanEditHandler;Accepts:Any
+            Marker;[];
+        CSharpCodeBlock - [0..19)::19


Still hate that we wrap things like this in code blocks but it's something we'll probably never change

ajaybhargavb assigned rynowak and NTaylorMullen Sep 27, 2018

ajaybhargavb requested review from NTaylorMullen and rynowak September 27, 2018 23:45

ajaybhargavb commented Sep 27, 2018

View reviewed changes

rynowak reviewed Sep 28, 2018

View reviewed changes

ajaybhargavb force-pushed the ajbaaska/razordirectivestest branch from e3517b0 to 15426c7 Compare October 17, 2018 21:54

ajaybhargavb force-pushed the feature/razor-parser branch from 5148739 to 9a862c6 Compare November 2, 2018 18:30

New baselines for RazorDirectivesTest

1b25ad9

ajaybhargavb force-pushed the ajbaaska/razordirectivestest branch from 15426c7 to 1b25ad9 Compare November 2, 2018 18:42

NTaylorMullen approved these changes Nov 2, 2018

View reviewed changes

ajaybhargavb merged commit 1b25ad9 into feature/razor-parser Nov 2, 2018

ajaybhargavb deleted the ajbaaska/razordirectivestest branch November 2, 2018 22:02

ajaybhargavb mentioned this pull request Dec 14, 2018

[Tracking] Parser improvements feedback dotnet/aspnetcore#5093

Closed

14 tasks

		@@ -1,7 +1,7 @@
		Markup span at (0:0,0 [2] ) (Accepts:Any) - Parent: Markup block at (0:0,0 [26] )
		Code span at (2:1,0 [2] ) (Accepts:Any) - Parent: Directive block at (2:1,0 [24] )

New baselines for RazorDirectivesTest #2621

New baselines for RazorDirectivesTest #2621

Uh oh!

Conversation

ajaybhargavb commented Sep 27, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants