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

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

Conversation

FrankRay78
Copy link
Contributor

@FrankRay78 FrankRay78 commented Oct 24, 2023

This PR fully addresses #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: #3291

This PR follows the same pattern introduced for the fix, which was located in Cake.Frosting.Internal.DefaultCommand, here:

private static CakeArguments CreateCakeArguments(IRemainingArguments remainingArguments, DefaultCommandSettings settings)

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. Unable to retrieve target argument with Frosting #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]

…he CakeArguments from CommandContext.Remaining, adding in the DefaultCommandSettings.Recompile flag, if necessary. Follows the same pattern already established in Cake.Frosting.Internal.DefaultCommand
@devlead devlead force-pushed the 4157-Upgrading-to-spectre.console-0.47.0-breaks-the-cake-build branch from 289be61 to 0da7e64 Compare October 30, 2023 07:42
Copy link
Member

@devlead devlead left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@devlead devlead merged commit 90836ff into cake-build:develop Oct 30, 2023
15 checks passed
@devlead
Copy link
Member

devlead commented Oct 30, 2023

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

@FrankRay78 FrankRay78 deleted the 4157-Upgrading-to-spectre.console-0.47.0-breaks-the-cake-build branch October 30, 2023 12:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to retrieve target argument with Frosting
2 participants