Added support for @attribute directive #607

ajaybhargavb · 2019-05-20T20:19:45Z

ajaybhargavb · 2019-05-20T20:23:36Z

src/Razor/src/Microsoft.AspNetCore.Razor.Language/Extensions/AttributeDirective.cs

+                throw new ArgumentNullException(nameof(builder));
+            }
+
+            builder.AddDirective(Directive, FileKinds.Legacy, FileKinds.Component, FileKinds.ComponentImport);


I assume we want this to work on non-blazor scenarios as well. I don't see why not.

src/Razor/src/Microsoft.AspNetCore.Razor.Language/Legacy/CSharpCodeParser.cs

NTaylorMullen · 2019-05-20T20:56:13Z

...zor/src/Microsoft.AspNetCore.Razor.Language/Extensions/DesignTimeDirectiveTargetExtension.cs

+            if (tokenKind == DirectiveTokenKind.Attribute)
+            {
+                // We don't need to do anything special here.
+                // We let the Roslyn take care of providing syntax errors for C# attributes.


This means you get 0 C# completion for attributes that don't make it into the generated C#. In practice we don't have to worry about this because our @attribute directive always gets represented in codegen; however, given that you added this to the extensible directive system anyone else extending is not guaranteed to have proper DesignTime bits generated for them.

Can you think of any way to include extensible directive attributes at this layer to ensure C# mappings are generated? If not I don't want to rat hole because in practice almost no one extends this portion of the Razor system; however, if it's easily addable It'd be worth doing.

I can't think of a simple way of doing this correctly. Because if I create a dummy class and put it on top of this, it might show have usage errors depending on the AttributeUsage of that attribute.

Alright, fair enough.

NTaylorMullen

once tests be green

ajaybhargavb · 2019-05-20T22:38:48Z

🆙 📅

rynowak · 2019-05-21T01:08:56Z

src/Razor/src/Microsoft.AspNetCore.Razor.Language/ComponentResources.resx

+    <value>Attribute</value>
+  </data>
+  <data name="AttributeDirective_Description" xml:space="preserve">
+    <value>Specifies the C# attribute that will be applied to the current document.</value>


This should say that it applies to the generated class.

I said document because that is what we say for @implements directive which also applies to the class. Should we change that to say class as well?

yeah I think so.

rynowak · 2019-05-21T01:09:19Z

/cc @SteveSandersonMS also @DamianEdwards who has been wanting this for some time 😁

rynowak · 2019-05-21T01:09:48Z

src/Razor/src/Microsoft.AspNetCore.Razor.Language/ComponentResources.resx

+    <value>Attribute</value>
+  </data>
+  <data name="AttributeDirective_Description" xml:space="preserve">
+    <value>Specifies the C# attribute that will be applied to the current document.</value>


This should say that it applies to the generated class. I think it's important to specifcially say that it goes on the class, even those that's obvious to us.

Same question as above.

src/Razor/src/Microsoft.AspNetCore.Razor.Language/Legacy/CSharpCodeParser.cs

rynowak · 2019-05-21T01:12:32Z

...isualStudioRazorParserIntegrationTest/ImpExprAcceptsDCIInStmtBlkAfterIdentifiers_1.stree.txt

@@ -19,7 +19,7 @@ RazorDocument - [0..22)::22 - [@{LF    @DateTime..LF}]
                                    Transition;[@];
                                CSharpImplicitExpressionBody - [9..19)::10
                                    CSharpCodeBlock - [9..19)::10
-                                        CSharpExpressionLiteral - [9..19)::10 - [DateTime..] - Gen<Expr> - ImplicitExpressionEditHandler;Accepts:NonWhitespace;ImplicitExpression[ATD];K21
+                                        CSharpExpressionLiteral - [9..19)::10 - [DateTime..] - Gen<Expr> - ImplicitExpressionEditHandler;Accepts:NonWhitespace;ImplicitExpression[ATD];K22


It's really annoything that these numbers show up in the diffs. Next time this comes up we should nuke that from orbit.

rynowak · 2019-05-21T01:13:13Z

src/Razor/test/RazorLanguage.Test/Legacy/RazorDirectivesTest.cs

+            // Act & Assert
+            ParseDocumentTest("@custom Serializable]",
+                new[] { descriptor });
+        }


What tests do we need for error cases?

Not sure what you mean. It creates a diag.txt with the error.

SteveSandersonMS · 2019-05-21T08:43:55Z

This was an amazingly quick response, @ajaybhargavb! Nice one :)

ajaybhargavb · 2019-05-21T21:19:49Z

@aspnet/build all checks have passed but the actual required one isn't green. Is this a known issue?

natemcmaster · 2019-05-21T21:23:14Z

I've seen this on other PRs too. Possible regression in the GitHub integration.

ajaybhargavb · 2019-05-21T21:24:26Z

The workaround is to re-run until it succeeds?

natemcmaster · 2019-05-21T21:24:52Z

Can you try a re-run just to see if this is a fluke?

ajaybhargavb · 2019-05-21T21:26:08Z

I requested a re-run but nothing seems to be happening.

natemcmaster · 2019-05-21T21:27:38Z

https://status.dev.azure.com/

ajaybhargavb · 2019-05-21T22:39:30Z

@aspnet/build, squash and merge for me please?

natemcmaster · 2019-05-21T22:42:49Z

Overriding GitHub requirements due to https://developercommunity.visualstudio.com/content/problem/576899/github-pull-request-does-not-report-a-check-for-th.html and merging.

ajaybhargavb · 2019-05-21T22:43:51Z

Thanks @natemcmaster

\n\nCommit migrated from dotnet/razor@67b8a83

\n\nCommit migrated from dotnet/razor@67b8a83 Commit migrated from dotnet/aspnetcore@72d017cc1b8e

ajaybhargavb requested review from NTaylorMullen and rynowak as code owners May 20, 2019 20:19

ajaybhargavb commented May 20, 2019

View reviewed changes

NTaylorMullen reviewed May 20, 2019

View reviewed changes

src/Razor/src/Microsoft.AspNetCore.Razor.Language/Legacy/CSharpCodeParser.cs Outdated Show resolved Hide resolved

NTaylorMullen reviewed May 20, 2019

View reviewed changes

NTaylorMullen approved these changes May 20, 2019

View reviewed changes

ajaybhargavb force-pushed the ajbaaska/attribute-directive branch from 3094cd5 to a4f7a53 Compare May 20, 2019 22:38

rynowak reviewed May 21, 2019

View reviewed changes

src/Razor/src/Microsoft.AspNetCore.Razor.Language/Legacy/CSharpCodeParser.cs Outdated Show resolved Hide resolved

rynowak reviewed May 21, 2019

View reviewed changes

ajaybhargavb added 4 commits May 21, 2019 13:20

Added support for @Attribute directive

cd11211

update

6ed11cd

Support multiline attributes

805ecec

updated documentation

3be9d08

ajaybhargavb force-pushed the ajbaaska/attribute-directive branch from 3e67f0f to 3be9d08 Compare May 21, 2019 20:32

natemcmaster merged commit 67b8a83 into master May 21, 2019

ghost deleted the ajbaaska/attribute-directive branch May 21, 2019 22:43

JunTaoLuo pushed a commit to dotnet/aspnetcore that referenced this pull request May 17, 2020

Added support for @Attribute directive (dotnet/razor#607)

72d017c

\n\nCommit migrated from dotnet/razor@67b8a83

dougbu pushed a commit to dougbu/razor-compiler that referenced this pull request Nov 17, 2021

Added support for @Attribute directive (dotnet/razor#607)

665132d

\n\nCommit migrated from dotnet/razor@67b8a83 Commit migrated from dotnet/aspnetcore@72d017cc1b8e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added support for @attribute directive #607

Added support for @attribute directive #607

ajaybhargavb commented May 20, 2019

ajaybhargavb May 20, 2019

rynowak May 21, 2019

NTaylorMullen May 20, 2019

ajaybhargavb May 20, 2019

NTaylorMullen May 20, 2019

NTaylorMullen left a comment

ajaybhargavb commented May 20, 2019

rynowak May 21, 2019

ajaybhargavb May 21, 2019

rynowak May 21, 2019

rynowak commented May 21, 2019

rynowak May 21, 2019

ajaybhargavb May 21, 2019

rynowak May 21, 2019

rynowak May 21, 2019

ajaybhargavb May 21, 2019

SteveSandersonMS commented May 21, 2019

ajaybhargavb commented May 21, 2019

natemcmaster commented May 21, 2019

ajaybhargavb commented May 21, 2019

natemcmaster commented May 21, 2019

ajaybhargavb commented May 21, 2019

natemcmaster commented May 21, 2019

ajaybhargavb commented May 21, 2019

natemcmaster commented May 21, 2019

ajaybhargavb commented May 21, 2019

Added support for @attribute directive #607

Added support for @attribute directive #607

Conversation

ajaybhargavb commented May 20, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NTaylorMullen left a comment

Choose a reason for hiding this comment

ajaybhargavb commented May 20, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rynowak commented May 21, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SteveSandersonMS commented May 21, 2019

ajaybhargavb commented May 21, 2019

natemcmaster commented May 21, 2019

ajaybhargavb commented May 21, 2019

natemcmaster commented May 21, 2019

ajaybhargavb commented May 21, 2019

natemcmaster commented May 21, 2019

ajaybhargavb commented May 21, 2019

natemcmaster commented May 21, 2019

ajaybhargavb commented May 21, 2019