-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Remove feature flags for #:include and #:exclude
#53775
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
Changes from 3 commits
c7668c2
6be0b80
c3185b6
c5abd10
7fdf23b
d1a16bc
2b0724d
232b4f2
242509c
efe50eb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -598,11 +598,6 @@ public enum IncludeOrExcludeKind | |
| /// </summary> | ||
| public sealed class IncludeOrExclude(in ParseInfo info) : Named(info) | ||
| { | ||
| public const string ExperimentalFileBasedProgramEnableIncludeDirective = nameof(ExperimentalFileBasedProgramEnableIncludeDirective); | ||
| public const string ExperimentalFileBasedProgramEnableExcludeDirective = nameof(ExperimentalFileBasedProgramEnableExcludeDirective); | ||
| public const string ExperimentalFileBasedProgramEnableTransitiveDirectives = nameof(ExperimentalFileBasedProgramEnableTransitiveDirectives); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do want to call out that I think the editor still needs changes to handle transitive directives properly. So today it will see
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am comfortable with moving forward on that change and just making sure we expedite it properly. But may want to extract this bit to separate PR, if our goal was to only remove experimental flags for these, for scenarios where we expect editor tooling to work.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @DamianEdwards what would you like to do about the transitive feature flag given IDE doesn't implement support for that yet?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @RikkiGibson what's the plan for integrating your entry-point detection changes? Is that the change you're referring to when you said:
I am concerned about missing the boat here a bit as the next train is 10.0.400 (or 11 previews) and that's not until August+. It would be super unfortunate if we're able to get the editor experience changes done well before then but we can't enable it by default in the SDK. That said, pulling the removal of flag on the transitive directive feature in the SDK into a separate PR seems like the prudent thing to do given the current state.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| public const string ExperimentalFileBasedProgramEnableItemMapping = nameof(ExperimentalFileBasedProgramEnableItemMapping); | ||
|
|
||
| public const string MappingPropertyName = "FileBasedProgramsItemMapping"; | ||
|
|
||
| public static string DefaultMappingString => ".cs=Compile;.resx=EmbeddedResource;.json=None;.razor=Content"; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -221,12 +221,10 @@ private ImmutableArray<CSharpDirective> EvaluateDirectives( | |
|
|
||
| internal ImmutableArray<(string Extension, string ItemType)> GetItemMapping(ProjectInstance project, ErrorReporter reportError) | ||
| { | ||
| return MSBuildUtilities.ConvertStringToBool(project.GetPropertyValue(CSharpDirective.IncludeOrExclude.ExperimentalFileBasedProgramEnableItemMapping)) | ||
| ? CSharpDirective.IncludeOrExclude.ParseMapping( | ||
| project.GetPropertyValue(CSharpDirective.IncludeOrExclude.MappingPropertyName), | ||
| EntryPointSourceFile, | ||
| reportError) | ||
| : CSharpDirective.IncludeOrExclude.DefaultMapping; | ||
| return CSharpDirective.IncludeOrExclude.ParseMapping( | ||
| project.GetPropertyValue(CSharpDirective.IncludeOrExclude.MappingPropertyName), | ||
| EntryPointSourceFile, | ||
| reportError); | ||
| } | ||
|
|
||
| public static ProjectInstance CreateProjectInstance( | ||
|
|
@@ -276,8 +274,6 @@ internal void CreateProjectInstance( | |
| evaluatedDirectives, | ||
| addGlobalProperties); | ||
|
|
||
| CheckDirectives(project, evaluatedDirectives, reportError); | ||
|
|
||
| return; | ||
| } | ||
|
|
||
|
|
@@ -325,8 +321,6 @@ internal void CreateProjectInstance( | |
| evaluatedDirectives = evaluatedDirectiveBuilder.ToImmutable(); | ||
| _evaluatedDirectives = (directivesOriginal, evaluatedDirectives); | ||
|
|
||
| CheckDirectives(project, evaluatedDirectives, reportError); | ||
|
|
||
| bool TryGetNextFileToProcess() | ||
| { | ||
| while (filesToProcess.TryDequeue(out var filePath)) | ||
|
|
@@ -399,51 +393,6 @@ ProjectRootElement CreateProjectRootElement(string projectFileText, ProjectColle | |
| } | ||
| } | ||
|
|
||
| private void CheckDirectives( | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So some version of this check, is going to return, when
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, and since I just merged the |
||
| ProjectInstance project, | ||
| ImmutableArray<CSharpDirective> directives, | ||
| ErrorReporter reportError) | ||
| { | ||
| bool? includeEnabled = null; | ||
| bool? excludeEnabled = null; | ||
| bool? transitiveEnabled = null; | ||
|
|
||
| foreach (var directive in directives) | ||
| { | ||
| if (directive is CSharpDirective.IncludeOrExclude includeOrExcludeDirective) | ||
| { | ||
| if (includeOrExcludeDirective.Kind == CSharpDirective.IncludeOrExcludeKind.Include) | ||
| { | ||
| CheckFlagEnabled(ref includeEnabled, CSharpDirective.IncludeOrExclude.ExperimentalFileBasedProgramEnableIncludeDirective, directive); | ||
| } | ||
| else | ||
| { | ||
| Debug.Assert(includeOrExcludeDirective.Kind == CSharpDirective.IncludeOrExcludeKind.Exclude); | ||
| CheckFlagEnabled(ref excludeEnabled, CSharpDirective.IncludeOrExclude.ExperimentalFileBasedProgramEnableExcludeDirective, directive); | ||
| } | ||
| } | ||
|
|
||
| if (directive.Info.SourceFile.Path != EntryPointSourceFile.Path) | ||
| { | ||
| CheckFlagEnabled(ref transitiveEnabled, CSharpDirective.IncludeOrExclude.ExperimentalFileBasedProgramEnableTransitiveDirectives, directive); | ||
| } | ||
| } | ||
|
|
||
| void CheckFlagEnabled(ref bool? flag, string flagName, CSharpDirective directive) | ||
| { | ||
| bool value = flag ??= MSBuildUtilities.ConvertStringToBool(project.GetPropertyValue(flagName)); | ||
|
|
||
| if (!value) | ||
| { | ||
| reportError( | ||
| directive.Info.SourceFile.Text, | ||
| directive.Info.SourceFile.Path, | ||
| directive.Info.Span, | ||
| string.Format(Resources.ExperimentalFeatureDisabled, flagName)); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| internal static void WriteProjectFile( | ||
| TextWriter writer, | ||
| ImmutableArray<CSharpDirective> directives, | ||
|
|
||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.
Was turning this single paragraph into two paragraphs intended?
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.
No, will revert, thanks.