Propogate additional compilation data from RoslynCompilationService #873
Conversation
| /// <param name="compiledCode">The contents that were compiled.</param> | ||
| /// <param name="messages">A sequence of <see cref="CompilationMessage"/> encountered during compilation.</param> | ||
| public CompilationFailedException( | ||
| [NotNull] string filePath, |
There was a problem hiding this comment.
we should also include the original razor text
|
Updated PR to include cshtml content. |
| } | ||
|
|
||
| public string GeneratedCode { get; private set; } | ||
| /// <summary> | ||
| /// Gets the path to the file that produced the compilation failure.. |
There was a problem hiding this comment.
Similar to fileContent / compiledCode separation, is this the .cshtml or .cs file?
There was a problem hiding this comment.
We never have a cs file, the cshtml is codegened in memory to produce C# content. In System.Web, these files had to be saved to disk since it was the only way to invoke csc.exe to produce a type.
There was a problem hiding this comment.
Updated to
Gets the path to the Razor source file that produced the compilation failure.
* Update CompilationFailedException to include path of file being compiled * Pass in path being compiled to Rolsyn. * Adding doc comments for compilation pieces Fixes #869
| { | ||
| try | ||
| { | ||
| using (var stream = file.CreateReadStream()) |
There was a problem hiding this comment.
nit: not sure if style cop catches this but should you have open and closing braces for this as well?
| @@ -35,8 +35,7 @@ public CompilationResult Compile([NotNull] IFileInfo file) | |||
| return _cache.GetOrAdd(file, () => CompileCore(file)); | |||
| } | |||
|
|
|||
| // TODO: Make this internal | |||
| public CompilationResult CompileCore(IFileInfo file) | |||
| internal CompilationResult CompileCore(IFileInfo file) | |||
There was a problem hiding this comment.
why are we making anything internal? I see the TODO but don't get why we'd execute on it prior to an all-up API review.
There was a problem hiding this comment.
The method's an implementation detail. It was public when we hadn't decided on using internal for unit testing.
| /// Gets the generated C# content that was compiled. | ||
| /// </summary> | ||
| /// <remarks> | ||
| /// For a Razor page, <c>ReadContent(compilationResult.File)</c> represents the actual contents on disk and |
There was a problem hiding this comment.
shoot. I didn't realize both File and ReadContent() were both private when I wrote my comment. How would something outside this class get the file content? is System.IO.File.ReadAllLines(FilePath) their best choice?
There was a problem hiding this comment.
I could leave the remark out since we're more explicit about what CompiledContent is and how it differs from the file content.
|
Updated PR. |
| /// Instantiates a new instance of <see cref="CompilationFailedException"/>. | ||
| /// </summary> | ||
| /// <param name="filePath">The path of the Razor source file that was compiled.</param> | ||
| /// <param name="fileContent">The contents of the Razor source file.</param> |
There was a problem hiding this comment.
looks like there's a few more instances of this nit / minor inconsistency ...
|
🚢 it after avoiding the short-term issue with the null propagating operator and fixing the bad .kproj merge. |
|
|
Fixes #869