Powderhouse subsystems and directives #2351

KathleenDollard · 2024-03-09T23:22:40Z

This addresses some of the issues raised in #2342 regarding directives (not response files environment variables(..

Remains in draft because:

There are more changes to the Tokenizer than I expected to evaluate directives separately - these changes extended to several files
I put some data onto the CLI Configuration, because I didn't know where to put it. When we determine how to carry information, we need to plan a way to tuck it away, probably similar to the SubSystem static class for subsystems.
A major piece of information is where the Tokenizer should begin. There are two reasons for this - the exe name may or may not be included in the args array and we need to deal with this, and Initialize methods for subsystems may have handled some of the args. We are limiting that handling to the start of the args array.
There are numerous naming and other issues to resolve, but I wanted to get the basics out for folks to look at before I am mostly busy next week.

KathleenDollard · 2024-03-10T11:46:23Z

This issue has files changed that I did not expect. I need to review this with Jean or Mikayla, regardless of content, and I know it makes it hard to review. So, unless you are in a patient mood, you can wait for a cleaner PR.

I did not deliberately delete most of the code marked as deleted.

KalleOlaviNiemitalo · 2024-03-10T12:19:51Z

The changes might be easier to understand if you updated the API compatibility test files too.

JeanJoeris

Paired with @KathleenDollard reviewing this. Left comments from review. Overall looks good, big remaining questions this review does not address are largely around:

StringExtensions / Tokenizer
- haven't reviewed as it is outdated / in flux
Handling of Directives as options
- conversations around this are currently in flight

src/System.CommandLine.Subsystems.Tests/DiagramSubsystemTests.cs

src/System.CommandLine.Subsystems.Tests/PipelineTests.cs

src/System.CommandLine.Subsystems.Tests/TestData.cs

src/System.CommandLine.Subsystems/Pipeline.cs

src/System.CommandLine.Subsystems/Subsystems/HandledInfo.cs

src/System.CommandLine.Subsystems/Subsystems/InitializationContext.cs

JeanJoeris · 2024-03-18T22:52:34Z

src/System.CommandLine/CliConfiguration.cs

+        public void MarkArgAsHandled(int argPos, string handledBy)
+        {
+
+        }


Do we want this empty, and is it related to the (possibly removed) HandledInfo?

src/System.CommandLine/CliConfiguration.cs

KathleenDollard · 2024-03-20T00:43:59Z

@KalleOlaviNiemitalo

The changes might be easier to understand if you updated the API compatibility test files too.

TBH, we are still getting everything to work with the split approach and have not given much thought to the API yet. We will do that a bit later.

I think the new subsystem approach will take some docs beyond API, and we're working on some design docs.

src/System.CommandLine.Subsystems/Subsystems/InitializationContext.cs

KathleenDollard · 2024-03-20T19:39:00Z

src/System.CommandLine.Tests/ParserTests.cs

@@ -94,48 +103,48 @@ public void Long_form_options_can_be_specified_using_equals_delimiter()
            result.GetResult(option).Tokens.Should().ContainSingle(a => a.Value == "there");
        }

-                [Fact]


It is not clear to me why whitespace is messed up here. Ideas? Maybe I found or created formatting nits and reformatted. Do we keep?

KathleenDollard · 2024-03-20T19:42:40Z

src/System.CommandLine/Parsing/CliParser.cs

@@ -148,11 +149,11 @@ string CurrentToken()

            configuration ??= new CliConfiguration(rootCommand);

-            CliTokenizer.Tokenize(
+            Tokenizer.Tokenize(


I am open to the CliToken name, but thought we might avoid it internal members

KathleenDollard · 2024-03-20T19:46:44Z

src/System.CommandLine/Parsing/StringExtensions.cs

@@ -27,10 +29,13 @@ internal static class StringExtensions
        */
    }

-    internal static class CliTokenizer
+    internal static class Tokenizer


Can we skip CLI for internals, or does it add meaning?

src/System.CommandLine/Parsing/StringExtensions.cs

* Preprocessed tokens can be skipped without messing up the location of subsequent tokens. * Tokens from response files can have accurate location information. This will enable better error handling and diagramming.

The configuration and args are now wrapped in an InitializationContext

The core parser no longer supports the special "directive" syntax, but this abstract class provides a reusable implementation of handling directive syntax in a subsystem using preprocessing.

mhutch changed the base branch from main to main-powderhouse March 13, 2024 19:37

JeanJoeris reviewed Mar 18, 2024

View reviewed changes

patriksvensson reviewed Mar 20, 2024

View reviewed changes

src/System.CommandLine.Subsystems/Subsystems/InitializationContext.cs Show resolved Hide resolved

KathleenDollard commented Mar 28, 2024

View reviewed changes

KathleenDollard marked this pull request as ready for review March 30, 2024 11:02

mhutch force-pushed the powderhouse-subsystem-fixes-and-directives branch from d4a316f to 9419c21 Compare April 4, 2024 04:37

KathleenDollard added 4 commits April 4, 2024 00:45

Tokenizer now tracks the location of tokens

032cb10

* Preprocessed tokens can be skipped without messing up the location of subsequent tokens. * Tokens from response files can have accurate location information. This will enable better error handling and diagramming.

Refactor subsystem initialization

83934e0

The configuration and args are now wrapped in an InitializationContext

Clean up pipeline/subsystem tests

0746536

Add abstract DirectiveSubsystem

8f7918d

The core parser no longer supports the special "directive" syntax, but this abstract class provides a reusable implementation of handling directive syntax in a subsystem using preprocessing.

mhutch force-pushed the powderhouse-subsystem-fixes-and-directives branch from 9419c21 to 8cc20e9 Compare April 4, 2024 04:46

KathleenDollard and others added 3 commits April 4, 2024 01:39

Add diagram subsystem and tests

9d3cbed

Add subsystem implementing response file handling

60f5272

Fix build on osx-arm64

06fa079

mhutch force-pushed the powderhouse-subsystem-fixes-and-directives branch from 8cc20e9 to 06fa079 Compare April 4, 2024 05:39

baronfel approved these changes Apr 4, 2024

View reviewed changes

KathleenDollard changed the title ~~Powderhouse directives~~ Powderhouse subsystems and directives Apr 4, 2024

KathleenDollard merged commit 84d40f3 into dotnet:main-powderhouse Apr 4, 2024
10 checks passed

KathleenDollard deleted the powderhouse-subsystem-fixes-and-directives branch April 4, 2024 20:13

KathleenDollard added the Powderhouse Work to isolate parser and features label Apr 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Powderhouse subsystems and directives #2351

Powderhouse subsystems and directives #2351

KathleenDollard commented Mar 9, 2024

KathleenDollard commented Mar 10, 2024

KalleOlaviNiemitalo commented Mar 10, 2024

JeanJoeris left a comment •

edited

Loading

JeanJoeris Mar 18, 2024

KathleenDollard commented Mar 20, 2024

KathleenDollard Mar 20, 2024

KathleenDollard Mar 20, 2024

KathleenDollard Mar 20, 2024

Powderhouse subsystems and directives #2351

Powderhouse subsystems and directives #2351

Conversation

KathleenDollard commented Mar 9, 2024

KathleenDollard commented Mar 10, 2024

KalleOlaviNiemitalo commented Mar 10, 2024

JeanJoeris left a comment • edited Loading

Choose a reason for hiding this comment

JeanJoeris Mar 18, 2024

Choose a reason for hiding this comment

KathleenDollard commented Mar 20, 2024

KathleenDollard Mar 20, 2024

Choose a reason for hiding this comment

KathleenDollard Mar 20, 2024

Choose a reason for hiding this comment

KathleenDollard Mar 20, 2024

Choose a reason for hiding this comment

JeanJoeris left a comment •

edited

Loading