Skip to content
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

Use System.CommandLine.Experimental instead of NDesk.Options #347

Merged

Conversation

@biohazard999
Copy link
Contributor

biohazard999 commented Sep 18, 2019

fixes #346

This is a major breaking change.

This PR replaces NDesk.Options with Microsoft's new System.CommandLine.Experimental.

Cause they are fundamentally different in architecture there are some key gotchas to the whole startup architecture as well as argument handling.

Motivation

  • be netstandard complient
  • use something that is supported by Microsoft
  • make it more extensible
  • allow plugin autors to add additional arguments to commands with ease
  • allow plugin authors to create custom commands

Before all commands shared a BaseParameters and CommandParameters class as well as the IHaveCommandLineOptions for extentions to provide custom arguments.

This changed fundamentally. Every command has it's own command line arguments. It is extensible via the CommandArgumentsExtentionAttribute and IHaveCommandLineArgs exports and interfaces.

Example for a new command:

[Export]
[Shared]
[CommandArguments(CommandName = BuiltInCommands.Import)]
public class ImportCommandParameters : PretzelBaseCommandParameters
{
        [ImportingConstructor]
        public ImportCommandParameters(IFileSystem fileSystem) : base(fileSystem) { }

        protected override void WithOptions(List<Option> options)
        {
            base.WithOptions(options);
            options.AddRange(new[]
            {
                new Option(new [] {"--importtype", "-i"}, "The import type")
                {
                    Argument = new Argument<string>()
                },
                new Option(new [] {"--importfile", "-f"}, "Path to import file")
                {
                    Argument = new Argument<string>()
                },
            });
        }

        public string ImportType { get; set; }

        public string ImportFile { get; set; }
}

[Shared]
[CommandInfo(CommandName = BuiltInCommands.Import, CommandDescription = "import posts from external source")]
    class ImportCommand : ICommand
{
  [Import]
  public ImportCommandParameters Parameters { get; set; }
}

CommandInfo.CommandName must match CommandArguments.CommandName to get hooked together.
To reduce risk of failure the BuiltInCommands class was introduced.

Every command that need's site context can use PretzelBaseCommandParameters as a base for the parameters. For extendable parameters authors can use BaseParameters as a base class. For complete control plugin authors can implement ICommandParameters and ICommandParametersExtendable on their own.

Plugin authors that need custom arguments for an command

Author's can extend multiple commands at once by using multiple command names:

   [Export]
    [Shared]
    [CommandArgumentsExtention(CommandNames = new[] { BuiltInCommands.Bake, BuiltInCommands.Taste })]
    public class VirtualDirectorySupportArguments : IHaveCommandLineArgs
    {
        public void UpdateOptions(IList<Option> options)
        {
            options.Add(new Option(new [] { "-vDir", "--virtualdirectory"}, "Rewrite url's to work inside the specified virtual directory")
            {
                Argument = new Argument<string>()
            });
        }

        public void BindingCompleted()
        {
        }

        public string VirtualDirectory { get; set; }
    }

The implementation keeps the same except for they just import their new parameters

  [Export(typeof(ITransform))]
    public class VirtualDirectorySupport : ITransform
    {
        [Import]
        public VirtualDirectorySupportArguments Arguments { get; set; }

        public void Transform(SiteContext siteContext)
        {
            if (string.IsNullOrEmpty(Arguments.VirtualDirectory)) return;
            /****/
      }
    }

New application startup

We use an separate RootCommand to provide the 3 parameters used in startup (majorly plugin discovery). Those get's injected afterwards into the actual rootcommand/subcommands after MEF kicked in. This way we get the benifit of having help available even if MEF discovery should fail in some sort of way. Attributes are processed before the actual command instance get's created. This way separate commands can't break the whole application if MEF binding will fail (at least for the commands, arguments are a different story).

The Logic from BaseParameters.SetPath moved to Program. Afterward's the -s parameter will be set. This way the value is correct before it hit's the arguments parsing. I think this simplifies the overall design of the system.
The reason for this parameter is: we can start pretzel with pretzel bake c:\foo\myblog hence no breaking change for pretzel consumers. There is probably a better way of solving this problem in the System.CommandLine library, but couldn't figure out how 🤷‍♂️.

Architectural questions that need to be discussed

  • Should Commands still be in Pretzel and should the parameters life there as well (or should we move them to Pretzel.Logic.
    • If Commands still should be there, should we provide an interface for the parameters that get's exported for plugin authors.
    • [] If not, how should plugin authors get needed parameters? We could for example double bind parameters (just use the same names as in the pretzel build in ones), but that could be error prown cause of the modifications that can be made in BindingComplete. Exporting for example the IBakeCommandParameters would be easy and intuitive.
  • IConfiguration got ReadFromFile method public, this way we get rid of the [Import("SourcePath")] and get a lot nicer lifecycle. It's now 100% clear when configuration get's read. So no weird side effects if IConfiguration get's read to early in the MEF lifecycle.
  • IHaveCommandLineArgs has a weird name. Probably we should rename it to ICommandArgumentsExtention that way it would match the attribute CommandArgumentsExtentionAttribute
  • Cause the new library supports async execution, I've changed the return type to Task but i think it would be befinitial to change it to Task<int> to support error codes, if we change it anyway.
  • I've obsoleted the Parameters.Path in favor of the Parameters.Source and Parameters.Destination properties, cause I think it's much clearer what path we are talking about. It's now also consistent with the command line parameter name.
  • I'm not very happy with the inconsistent naming with arguments and parameters so we should be consistent here, cause when we break anyway it should be consistent.

Things that need to be done before merging

  • Get test converage up
  • Make sure we don't loose functionality
  • Make old unit tests work
    • Check if we don't miss any important tests
    • Test parameters for extentions
    • Test PretzelCommandHandler
    • Test CommandCollection
    • Test arguments hook in Program.cs
    • IntegrationTest composition
  • Document breaking changes

@laedit as always, would be cool if you got an early eye on this. I think this layes out a good foundation for the future of 🥨 ❤

This is an opener for #146

biohazard999 added 23 commits Sep 4, 2019
Dotliquid sorting (#325)
@biohazard999 biohazard999 marked this pull request as ready for review Sep 23, 2019
@laedit

This comment has been minimized.

Copy link
Member

laedit commented Sep 24, 2019

Thanks a lot for your hard work!
I am sorry but I will probably don't have the time to check it before this weekend...

@laedit

This comment has been minimized.

Copy link
Member

laedit commented Oct 5, 2019

So sorry that it took me so long, but I needed to try some things about System.CommandLine to understand it and be comfortable to review your PR, you did an excellent job!
That said there are some small things that I think can be simplified (and more with the #353), I will post some comments.

src/Pretzel/Program.cs Outdated Show resolved Hide resolved
src/Pretzel/Program.cs Show resolved Hide resolved
src/Pretzel/Program.cs Outdated Show resolved Hide resolved
src/Pretzel/Program.cs Outdated Show resolved Hide resolved
src/Pretzel/Program.cs Show resolved Hide resolved
src/Pretzel/Program.cs Outdated Show resolved Hide resolved
src/Pretzel/Commands/CommandCollection.cs Outdated Show resolved Hide resolved
src/Pretzel/Commands/CommandCollection.cs Outdated Show resolved Hide resolved
V1.0 - biohazard automation moved this from In progress to Review in progress Oct 5, 2019
V1.0 - biohazard automation moved this from Review in progress to Reviewer approved Oct 8, 2019
@laedit
laedit approved these changes Oct 8, 2019
@laedit

This comment has been minimized.

Copy link
Member

laedit commented Oct 8, 2019

We have finally pulled this through!
Again, thanks a lot for your hard work and having endured me 😄

@biohazard999

This comment has been minimized.

Copy link
Contributor Author

biohazard999 commented Oct 8, 2019

We have finally pulled this through!

🎉🥳

Again, thanks a lot for your hard work and having endured me 😄

It's totally fine! Nothing to worry about 😁 A couple more PR's and we have a nice dotnet global tool 🥰

@laedit laedit merged commit 48729e0 into Code52:master Oct 8, 2019
1 check passed
1 check passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
V1.0 - biohazard automation moved this from Reviewer approved to Done Oct 8, 2019
@biohazard999 biohazard999 deleted the biohazard999:topic/System.CommandLine.Experimental-346 branch Oct 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
2 participants
You can’t perform that action at this time.