System command line full upgrade #120
Conversation
Using default help builder.
Adding releasify command
Started writing some tests to validate the CLI parsing.
|
Just copying what I posted to Discord for visibility: Just saw your latest push to the PR. I thought I had mentioned this already but perhaps not. I think we should probably leave the Update.Windows command line alone. There are lots of electron/non-C# apps that rely on this API and I'm not sure I want to make breaking changes to it at this point. Happy to discuss but my general feeling is: Breaking the CLI tool? fine. Breaking an installed app because they didn't read the docs when upgrading to a newer version? Probably less fine. |
Keboo
force-pushed
the
SystemCommandLine-FullUpgrade
branch
from
5e0624b to
b6cbcff
Compare
Starting on Windows command tests
Fixing up validation messages to display the alias that was used
Keboo
changed the title
| public class ReleaseCommand : SigningCommand | ||
| { | ||
| public Option<Uri> BaseUrl { get; } | ||
| public Option<string> AddSearchPath { get; } |
There was a problem hiding this comment.
This only has an arity of 1, but should it have an arity of 0 to many?
There was a problem hiding this comment.
This argument as well as --mainExe (windows only) should allow 0 to many. Additionally --addSearchPath probably makes more sense as a global option.
There was a problem hiding this comment.
I updated both of these to both have an arity of 0 to many (this is the default for options of arrays).
I am not sure what is meant by making --addSearchPath a global option. Do you mean allow it to be specified for any command?
There was a problem hiding this comment.
Yes, --addSearchPath should probably be available for any command. If you are providing updated/custom binaries (eg. 7z, rcedit, wix, signtool) you can put them in a folder, and add that folder to the search path list, and Squirrel will find your version first instead of the bundled version.
| } | ||
| } | ||
| }); | ||
| PackageContent.ArgumentHelpName = "key=<FILE>"; |
There was a problem hiding this comment.
Is this a reasonable way to display to the user how to specify the package content?
There was a problem hiding this comment.
In Mono.Options, the way you specify this was --pkgContent welcome mywelcomefile.html. Assuming an = is now required I guess this is fine.
There was a problem hiding this comment.
It could be made to work that way, but Mono.Options relies much more on the position of the raw arguments, while System.CommandLine allows for specifying things in any reasonable order, and matching tokens up with the relevant options/commands. We could make the original format work, but it gets complicated because of the space, terminals will treat those as separate tokens. Having something like an = or : will link the values together. Alternatively we could always allow for spaces between them if the caller knew to quote the argument value (ie. --pkgContent "welcome mywelcomefile.html")
There was a problem hiding this comment.
How would this new parsing handle paths with quotes and spaces? eg. --pkgContent welcome="folder name/mywelcomefile.html" ?
|
I believe this is ready for review. |
|
Thanks for your hard work on this! I should have time to go through this a bit later this week. |
| { | ||
| public class PackCommand : BaseCommand | ||
| { | ||
| public Option<string> PackId { get; } |
There was a problem hiding this comment.
Presumably this could share a base class with the windows pack/releasify command as there are a lot of shared arguments. Particularly these: PackId, PackVersion, PackDirectory, PackAuthors, PackTitle, IncludePdb, ReleaseNotes, NoDelta.
SquirrelAwareExecutable is different in mac/windows. On windows this can be provided more than once, on mac it must be only specified once.
There was a problem hiding this comment.
It certainly could, I was a bit torn when I did this on if they should share a base class or not. I am happy to refactor this so that they are shared. I wasn't sure how much was similar by necessity vs coincidence.
There was a problem hiding this comment.
The specific arguments I mentioned are functionally equivalent in both OSX and Windows. They are both passed into NugetConsole.CreatePackageFromMetadata. I'm sure we could refactor parsing options & calling this function with some shared POCO to remove some boilerplate in the future. Happy to accept PR with or without this refactor but something to keep on the radar. The other similar options (eg. mainExe, icon etc) are different enough they should probably remain separate.
|
|
||
| if (options.icon == null || !File.Exists(options.icon)) | ||
| throw new OptionValidationException("--icon is required when generating a new app bundle."); | ||
| throw new ArgumentException("--icon is required when generating a new app bundle."); |
There was a problem hiding this comment.
Is there a way to get the command alias from System.CommandLine so that "--icon" is not hard-coded here?
There was a problem hiding this comment.
There are a couple of ways to do this. The first is to simply grab the alias for the option (Generally grabbing the longest one):
new PackCommand().Icon.Aliases.OrderByDescending(x => x.Length).First();
There is also a Name property on the Options, but this property omits the prefix (- or --).
IMO a better solution is to use whichever alias the caller specified when invoking the CLI, as this feel more natural for an error message. However to know which alias was specified requires inspecting the ParseResult. This means prompting the user with the error while the command line is parsed rather than doing the validation after the command is invoked. You can see several examples of this in the custom validation that I put inside of the SystemCommandLineExtensions.
I suspect many of these failure cases could be tested for during the parse options (and in fact the icon one should already be covered). So this validation checking could be removed. I had just done my best to avoid touching it because the PR was already getting large.
|
Is it possible to reduce the maximum character wrap length in the help text to be consistent with Mono.Options? (80 char). I had a look at replacing |
Yea that API should have worked the way you expected. There is a bug in the current release that ignores the requested maxWidth and uses the terminal's size instead. I have put in a workaround for this, and submitted a PR to fix the underlying issue. |
I wouldn't mind this behaviour (auto sizing to terminal instead of hard-coded) but it didn't work this way for me. It ended up always setting the width a bit too wide so all the lines wrapped awkwardly until I enlarged my terminal. |
2 participants
I wanted to open this so you could see the current progress and provide any feedback. I still have more work to do on it, but this should give you a decent idea of the direction I am heading down.