-
Notifications
You must be signed in to change notification settings - Fork 1.2k
File-based apps: add support for #:include
#52347
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: release/10.0.2xx
Are you sure you want to change the base?
Conversation
bf9dc98 to
d13a3f2
Compare
d13a3f2 to
c06c1fc
Compare
c06c1fc to
d64bb1a
Compare
| VirtualProjectBuilder.WriteProjectFile( | ||
| csprojWriter, | ||
| directives, | ||
| evaluatedDirectives, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've noticed the run-api doesn't evaluate directives, so I've fixed that, and added tests for it.
| <value>Unrecognized directive '{0}'.</value> | ||
| <comment>{0} is the directive name like 'package' or 'sdk'.</comment> | ||
| </data> | ||
| <data name="EmptyTempPath" xml:space="preserve"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not used in the package, so moved out.
|
|
||
| public readonly struct ParseInfo | ||
| { | ||
| public required SourceFile SourceFile { get; init; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to know source file that directives come from to evaluate relative paths in #:include/#:exclude directives, so moved this from ParseContext to ParseInfo.
| } | ||
|
|
||
| internal delegate void ErrorReporter(SourceFile sourceFile, TextSpan textSpan, string message); | ||
| internal delegate void ErrorReporter(SourceFile sourceFile, TextSpan textSpan, string message, Exception? innerException = null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The inner exception used to be passed but got lost in this refactoring: #51995, so I've restored it.
| } | ||
|
|
||
| public static void RemoveDirectivesFromFile(ImmutableArray<CSharpDirective> directives, SourceText text, string filePath) | ||
| public static void RemoveDirectivesFromFile(ImmutableArray<CSharpDirective> directives, SourceFile sourceFile, string targetFilePath) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function did not used to save the file if not modified - I don't think that was intentional but it worked thanks to the project convert command copying all Compile items anyway. It all works differently (better) now.
|
|
||
| private static void Convert(string inputCSharp, out string actualProject, out string? actualCSharp, bool force, string? filePath, | ||
| bool collectDiagnostics, out ImmutableArray<SimpleDiagnostic>.Builder? actualDiagnostics) | ||
| private static void Convert( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unified the helpers here.
| (".cs", "Compile"), | ||
| (".resx", "EmbeddedResource"), | ||
| (".json", "Content"), | ||
| (".razor", "Content"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interested in thoughts RE other default file types to support here. I think definitely *.cshtml to go along with *.razor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we could easily add extensibility for this. Someone (user/sdk/etc) could define a property like <FileBasedProgramsItemMapping>.protobuf=Protobuf;.cshtml=Content</>). We have access to a ProjectInstance when resolving these, so it should be simple to implement:
sdk/src/Microsoft.DotNet.ProjectTools/VirtualProjectBuilder.cs
Lines 180 to 182 in d64bb1a
| // NOTE: In the future, instead of using only hard-coded item types here, | |
| // we could read some user/sdk-defined MSBuild property from `project` for additional mapping. | |
| includeOrExcludeDirective = includeOrExcludeDirective.WithDeterminedItemType(reportError); |
Just needs some design I guess.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah my intent was that this would be extensible, and be called something like "implicit item types". Not required to ship first version of support IMO but if we have time then great. If you think it's easy enough to do though happy for us to aim on implementing it that way and moving the declarations into the respective SDKs/packages.
Resolves #48174.
Fixes #50912.