search

cake-build / cake

:cake: Cake (C# Make) is a cross platform build automation system.
https://cakebuild.net
MIT License
3.84k stars 722 forks source link

Upgrade spectre.console to 0.47.0, modifying the Cake codebase to fix a breaking change this new version includes #4236

Closed FrankRay78 closed 8 months ago

FrankRay78 commented 8 months ago

This PR fully addresses https://github.com/cake-build/cake/issues/4157, which was preventing spectre.console from being upgraded to 0.47.0. The PR also includes the actual upgrade to spectre.console 0.47.0

The approach taken is very similar to a previous Cake.Frosting issue, which has already been fixed: https://github.com/cake-build/cake/issues/3291

This PR follows the same pattern introduced for the fix, which was located in Cake.Frosting.Internal.DefaultCommand, here: https://github.com/cake-build/cake/blob/c3cd2af8d73b4f270d6abe585c41f049ea92a0d7/src/Cake.Frosting/Internal/Commands/DefaultCommand.cs#L82

Within Cake proper (rather than Cake.Frosting)... Cake.Commands.DefaultCommand.Execute(...) has been modified to explicitly create the CakeArguments from CommandContext.Remaining, adding in the DefaultCommandSettings.Recompile flag, if necessary.

This PR has indirectly resulted in a move towards to using CakeArguments rather than the spectre.console IRemainingArgument, which seems a lot cleaner and self-contained.

Full disclosure: I'm a little uncomfortable submitting a PR with no unit tests, however my reasons for doing it are as follows:

  1. DefaultCommand, both Cake or Cake.Frosting ones, have entirely zero unit tests (rather, they seem to represent the top level container, and as such aren't tested)
  2. https://github.com/cake-build/cake/issues/3291, which this PR was modelled on, didn't see the need to introduce unit tests to cover the IRemainingArguments behaviour
  3. It's not entirely clear whether this is even the right level for unit testing

However, and relevant to both Cake and Cake.Frosting DefaultCommand's, I would consider introducing tests for this argument handling behaviour, if deemed necessary.

[It might be some kind of factory to create the CakeArguments, added into the IServiceCollection, and injected into the DefaultCommand, two versions of factory I guess, one for Cake and one for Cake.Frosting (given both commands have different DefaultCommandSettings classes). But that's just some thinking off the top of my head]

devlead commented 8 months ago

@FrankRay78 your changes have been merged, thanks for your contribution 👍