- 
                Notifications
    You must be signed in to change notification settings 
- Fork 411
Separate parse from invocation configurations #2606
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
Separate parse from invocation configurations #2606
Conversation
| I was going to comment that a suggest directive or a help action would need to be able to read the parsing configuration at invocation time, such as EnablePosixBundling. But I see ParseResult does have public properties for both configurations. | 
CommandLineConfigurationThere 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.
Overall it LGTM, but please see my comments with some suggestions for improvements. Thank you @jonsequitur !
        
          
                ...ility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                ...ility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt
              
                Outdated
          
            Show resolved
            Hide resolved
        
      775d19a    to
    1f37d7d      
    Compare
  
    Co-authored-by: Adam Sitnik <[email protected]>
c257047    to
    24aed51      
    Compare
  
    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 am hitting the approve button to get you unblocked @jonsequitur , but please address my comment about Invoke before merging. Thanks a lot!
        
          
                ...ility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt
          
            Show resolved
            Hide resolved
        
              
          
                ...ility.Tests/ApiCompatibilityApprovalTests.System_CommandLine_api_is_not_changed.approved.txt
              
                Outdated
          
            Show resolved
            Hide resolved
        
      f3c2633    to
    faee41a      
    Compare
  
    | @jonsequitur, @adamsitnik, @jefferson-lam: This change in the contract between 2.0.0-beta6 and 2.0.0-beta7 should be documented as it was when breaking changes were introduced in 2.0.0-beta5. https://learn.microsoft.com/en-us/dotnet/standard/commandline/migration-guide-2.0.0-beta5 I had to dig into the code to find what had been changed. | 
| Hope this helps someone who is having the same problem. Before After  | 
This PR's goal is to separate the
CommandLineConfigurationclass into two separate classes, one of which configures parsing behaviors (now calledParserConfiguration) and the other of which configures invocation behaviors (InvocationConfiguration).InvocationConfigurationhas four properties which were moved over fromCommandLineConfiguration:EnableDefaultExceptionHandlerProcessTerminationTimeoutOutputErrorI've also removed the
Parse,Invoke, andInvokeAsyncmethods from the formerCommandLineConfiguration, since they're also available onParseResult. The redundancy seemed unnecessary, although these invoke methods did include aParsecall, making the calling pattern more like the old, more succinctCommand.Invokemethods.Next steps:
RootCommandto theCommandLineConfigurationconstructor should not be necessary, and can lead to some unclear situations, such as: