Refactored CodeGeneratorContext to CodeBuilderContext. #114

NTaylorMullen · 2014-09-04T22:15:14Z

Needed to separate the context's of "generation" and "building" to enable the communication of the TagHelperProvider.

rynowak · 2014-09-05T19:09:27Z

src/Microsoft.AspNet.Razor/ParserResults.cs

@@ -34,6 +39,11 @@ protected ParserResults(bool success, Block document, IList<RazorError> errors)
        public Block Document { get; private set; }

        /// <summary>
+        /// The tag helper management object used to maintain found tag helpers during parsing.
+        /// </summary>
+        public TagHelperProvider TagHelperProvider { get; private set; }


This really seems service-like. Why does it need to be threaded through all of these layers? Why is it part of the results

Razor doesn't use any sort of DI sadly, therefore this is how end-information is passed.

I'm trying to think of an alternate use of this object "outside" of generating C# code and depending on how tooling implements their portion of TagHelpers they "MAY" need access to the TagHelperProvider from the parser results, that being said though I see your point. We need a way to get the TagHelperProvider "out" of the parser and the portion of the Parser that will create the TagHelperProvider must be here: https://github.com/aspnet/Razor/blob/CodeBuilderContext_Refactor/src/Microsoft.AspNet.Razor/Parser/RazorParser.cs#L146 OR the last Optimizer.

We pass other information out of the parser like the Document that's needed only code gen and tooling and is really just flowed throughout the application (much like the tag helper provider).

Do you see a better approach to getting this data out of the parser and making it still available to tooling/the code builder (definitely open to suggestions)?

suggest carting this reference around in a context object or passing it directly. Either way, limit the Results classes to actual results

There's already a context that the parser uses and initializes in its parse method (dependent on its own member values).
Options:

Add an out parameter to the parse method that "provides" the TagHelperProvider.

Add callback for TagHelperProvider (pretty much just like an out param but not an out param lol).

Pass in a TagHelperProvider builder of some sort into the RazorParser's ctor and use it to create the TagHelperProvider when parse is called. We can then pull from the builder.

Expose the TagHelperProvider on the RazorParser itself.

Leave it on ParserResults.

As a part of Tag Helpers: Add @removetaghelper directive and link to registration system #112 and Tag Helpers: Add @addtaghelper directive and link to registration system #111 a tag helper locator can be passed into the ctor which can then be utilized to fetch the TagHelperProvider off of. As a temporary fix for this PR until those issues are completed I can provide a dummy implementation of it.

observations:

since nothing in this PR dereferences the TagHelperProvider instance, its addition seems easily separated from the refactoring done in this review

that'll make this review much less controversial

when we start passing around a TagHelperProvider, the first questions will be "who needs this?" and "where will it come from?" with that info, we can work through how best to make the instance available.

Not sure I follow your numbering.

The goal is to communicate the RazorParser generated TagHelperProvider to the CodeBuilderContext that's used by the CSharpCodeBuilder to render TagHelpers. For now I can remove this however. The CodeBuilderContext's TagHelperProvider will be empty for the interim

NTaylorMullen · 2014-09-06T03:19:27Z

Addressed a "few" code review comments, not the "bigger" design questions still mullying around.

NTaylorMullen · 2014-09-06T05:56:53Z

Modify TagHelperAttributeCodeGeneratorTest to use CodeBuilderContext.

dougbu · 2014-09-06T22:42:07Z

src/Microsoft.AspNet.Razor/Generator/CodeBuilderContext.cs

@@ -0,0 +1,20 @@
+using System;


copyright header

dougbu · 2014-09-06T23:06:00Z

⌚

NTaylorMullen · 2014-09-07T08:08:14Z

Addressed code review comments except for: #114 (comment)

dougbu · 2014-09-07T20:23:03Z

src/Microsoft.AspNet.Razor/Generator/CodeBuilderContext.cs

+        /// <summary>
+        /// Instantiates a new instance of the <see cref="CodeBuilderContext"/> object.
+        /// </summary>
+        /// <param name="generatorContext">A <see cref="CodeGeneratorContext"/> copy information from.</param>


"copy" -> "to copy"

dougbu · 2014-09-07T20:37:32Z

test/Microsoft.AspNet.Razor.Test/CSharpRazorCodeLanguageTest.cs

+                                                            string.Empty,
+                                                            shouldGenerateLinePragmas: false);
+            var codeBuilderContext = new CodeBuilderContext(
+                generatorContext, new TagHelperProvider(Enumerable.Empty<TagHelperDescriptor>()));


need an internal test-only constructor for CodeBuilderContext that avoids need to create a base CodeGeneratorContext just to instantiate the CodeBuilderContext

dougbu · 2014-09-07T20:46:51Z

⌚

NTaylorMullen · 2014-09-08T03:09:35Z

Addressed code review comments except for outstanding issue.

NTaylorMullen · 2014-09-10T03:28:57Z

Addressed all CR comments.

NTaylorMullen · 2014-09-10T03:30:10Z

src/Microsoft.AspNet.Razor/Parser/ParserContext.cs

@@ -9,6 +9,7 @@
 using System.Linq;


Undo changes to this file

- Needed to separate the context's of "generation" and "building" to enable the communication of the TagHelperProvider.

- Removed tag helper provider from parser results and codebuildercontext.

dougbu · 2014-09-11T05:05:24Z

src/Microsoft.AspNet.Razor/Generator/CodeBuilderContext.cs

+        /// If <see cref="TargetWriterName"/> is <c>null</c> values will be written using a default write method
+        /// i.e. WriteLiteral("Hello World").
+        /// If <see cref="TargetWriterName"/> is not <c>null</c> values will be written to the given 
+        /// <see cref="TargetWriterName"/>, i.e. WriteLiteralTo(myWriter, "Hello World").


"i.e." -> "e.g."

dougbu · 2014-09-11T05:08:53Z

w/ the one small comment fix

NTaylorMullen force-pushed the TagHelpers_ParseTreeRewriterTests branch from f75b598 to ebd69e8 Compare September 5, 2014 02:56

NTaylorMullen force-pushed the CodeBuilderContext_Refactor branch 2 times, most recently from 68c434a to 8476d27 Compare September 5, 2014 03:16

NTaylorMullen force-pushed the TagHelpers_ParseTreeRewriterTests branch from ebd69e8 to 84e9e15 Compare September 5, 2014 07:15

NTaylorMullen force-pushed the CodeBuilderContext_Refactor branch from 8476d27 to 96c4089 Compare September 5, 2014 07:15

rynowak reviewed Sep 5, 2014
View reviewed changes

NTaylorMullen force-pushed the TagHelpers_ParseTreeRewriterTests branch from 84e9e15 to 5f8ef5e Compare September 6, 2014 03:17

NTaylorMullen force-pushed the CodeBuilderContext_Refactor branch from 96c4089 to 89a5146 Compare September 6, 2014 03:18

dougbu reviewed Sep 6, 2014
View reviewed changes

src/Microsoft.AspNet.Razor/Generator/CodeBuilderContext.cs

@@ -0,0 +1,20 @@

using System;

Copy link

Member

dougbu Sep 6, 2014

Choose a reason for hiding this comment

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

copyright header

NTaylorMullen force-pushed the TagHelpers_ParseTreeRewriterTests branch from 5f8ef5e to 8145a8f Compare September 7, 2014 06:47

NTaylorMullen mentioned this pull request Sep 7, 2014

Add XML comments to visible Razor infrastructure #118

Closed

NTaylorMullen force-pushed the CodeBuilderContext_Refactor branch from 89a5146 to 58f1d1f Compare September 7, 2014 08:07

dougbu reviewed Sep 7, 2014
View reviewed changes

NTaylorMullen force-pushed the TagHelpers_ParseTreeRewriterTests branch from 8145a8f to fb1befb Compare September 8, 2014 02:51

NTaylorMullen force-pushed the CodeBuilderContext_Refactor branch from 58f1d1f to bbca396 Compare September 8, 2014 03:09

NTaylorMullen force-pushed the TagHelpers_ParseTreeRewriterTests branch from fb1befb to e6e30f6 Compare September 9, 2014 00:47

NTaylorMullen force-pushed the CodeBuilderContext_Refactor branch from bbca396 to 3a0c399 Compare September 9, 2014 00:50

NTaylorMullen force-pushed the TagHelpers_ParseTreeRewriterTests branch from e6e30f6 to 52411e9 Compare September 9, 2014 21:19

NTaylorMullen force-pushed the CodeBuilderContext_Refactor branch from 3a0c399 to 458982d Compare September 9, 2014 21:29

NTaylorMullen reviewed Sep 10, 2014
View reviewed changes

src/Microsoft.AspNet.Razor/Parser/ParserContext.cs

@@ -9,6 +9,7 @@

using System.Linq;

Copy link

Contributor Author

NTaylorMullen Sep 10, 2014

Choose a reason for hiding this comment

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

Undo changes to this file

NTaylorMullen force-pushed the TagHelpers_ParseTreeRewriterTests branch from 52411e9 to a49a010 Compare September 10, 2014 23:38

NTaylorMullen added 4 commits September 10, 2014 16:38

Refactored CodeGeneratorContext to CodeBuilderContext.

28cae8c

- Needed to separate the context's of "generation" and "building" to enable the communication of the TagHelperProvider.

Rebased.

79ddba2

Addressed code review comments

5d0cec7

- Removed tag helper provider from parser results and codebuildercontext.

Addressed code review comments.

9711c9e

NTaylorMullen force-pushed the CodeBuilderContext_Refactor branch from 58a885b to 9711c9e Compare September 10, 2014 23:47

dougbu reviewed Sep 11, 2014
View reviewed changes

NTaylorMullen closed this Sep 11, 2014

NTaylorMullen deleted the CodeBuilderContext_Refactor branch September 11, 2014 21:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactored CodeGeneratorContext to CodeBuilderContext. #114

Refactored CodeGeneratorContext to CodeBuilderContext. #114

NTaylorMullen commented Sep 4, 2014

rynowak Sep 5, 2014

NTaylorMullen Sep 6, 2014

NTaylorMullen Sep 6, 2014

dougbu Sep 6, 2014

NTaylorMullen Sep 7, 2014

dougbu Sep 7, 2014

NTaylorMullen Sep 8, 2014

NTaylorMullen commented Sep 6, 2014

NTaylorMullen commented Sep 6, 2014

dougbu Sep 6, 2014

dougbu commented Sep 6, 2014

NTaylorMullen commented Sep 7, 2014

dougbu Sep 7, 2014

dougbu Sep 7, 2014

dougbu commented Sep 7, 2014

NTaylorMullen commented Sep 8, 2014

NTaylorMullen commented Sep 10, 2014

NTaylorMullen Sep 10, 2014

dougbu Sep 11, 2014

dougbu commented Sep 11, 2014

Refactored CodeGeneratorContext to CodeBuilderContext. #114

Refactored CodeGeneratorContext to CodeBuilderContext. #114

Conversation

NTaylorMullen commented Sep 4, 2014

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NTaylorMullen commented Sep 6, 2014

NTaylorMullen commented Sep 6, 2014

Choose a reason for hiding this comment

dougbu commented Sep 6, 2014

NTaylorMullen commented Sep 7, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dougbu commented Sep 7, 2014

NTaylorMullen commented Sep 8, 2014

NTaylorMullen commented Sep 10, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dougbu commented Sep 11, 2014