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.

Sign up for GitHub

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

Jump to bottom

upgrade: Make --keep-going the default #308

Merged
merged 4 commits into from
Feb 18, 2022
Merged

upgrade: Make --keep-going the default #308

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b1466dd
upgrade: Make --keep-going the default
autoantwort Dec 21, 2021
5a35699
Merge remote-tracking branch 'origin/main' into make-upgrade---keep-g…
BillyONeal Feb 12, 2022
530c64c
Apply code review feedback.
BillyONeal Feb 12, 2022
9f54e58
Add message.
BillyONeal Feb 12, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 13 additions & 3 deletions src/vcpkg/commands.upgrade.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,13 @@ namespace vcpkg::Commands::Upgrade

static constexpr StringLiteral OPTION_NO_DRY_RUN = "no-dry-run";
static constexpr StringLiteral OPTION_KEEP_GOING = "keep-going";
static constexpr StringLiteral OPTION_STOP_AT_FAILURE = "stop-at-failure";
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
static constexpr StringLiteral OPTION_STOP_AT_FAILURE = "stop-at-failure";
static constexpr StringLiteral OPTION_NO_KEEP_GOING = "no-keep-going";

this is the standard.

static constexpr StringLiteral OPTION_ALLOW_UNSUPPORTED_PORT = "allow-unsupported";

static constexpr std::array<CommandSwitch, 3> INSTALL_SWITCHES = {{
static constexpr std::array<CommandSwitch, 4> INSTALL_SWITCHES = {{
{OPTION_NO_DRY_RUN, "Actually upgrade"},
{OPTION_KEEP_GOING, "Continue installing packages on failure"},
{OPTION_KEEP_GOING, "The default (deprecated)"},
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
{OPTION_KEEP_GOING, "The default (deprecated)"},
{OPTION_KEEP_GOING, "Continue installing packages on failure (default)"},

{OPTION_STOP_AT_FAILURE, "Stop installing packages on failure"},
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
{OPTION_STOP_AT_FAILURE, "Stop installing packages on failure"},
{OPTION_NO_KEEP_GOING, "Stop installing packages on failure"},

{OPTION_ALLOW_UNSUPPORTED_PORT, "Instead of erroring on an unsupported port, continue with a warning."},
}};

Expand All @@ -54,8 +56,16 @@ namespace vcpkg::Commands::Upgrade

const ParsedArguments options = args.parse_arguments(COMMAND_STRUCTURE);

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, make sure that you check for both OPTION_KEEP_GOING and OPTION_NO_KEEP_GOING and error if both are passed.

if (Util::Sets::contains(options.switches, OPTION_KEEP_GOING))
{
print2(Color::warning,
"The option `",
OPTION_KEEP_GOING,
"` is the default now. You don't have to pass this option anymore.");
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (Util::Sets::contains(options.switches, OPTION_KEEP_GOING))
{
print2(Color::warning,
"The option `",
OPTION_KEEP_GOING,
"` is the default now. You don't have to pass this option anymore.");
}

it's not standard practice to warn on passing the "default" for an option.

const bool no_dry_run = Util::Sets::contains(options.switches, OPTION_NO_DRY_RUN);
const KeepGoing keep_going = to_keep_going(Util::Sets::contains(options.switches, OPTION_KEEP_GOING));
const KeepGoing keep_going = to_keep_going(!Util::Sets::contains(options.switches, OPTION_STOP_AT_FAILURE));
const auto unsupported_port_action = Util::Sets::contains(options.switches, OPTION_ALLOW_UNSUPPORTED_PORT)
? Dependencies::UnsupportedPortAction::Warn
: Dependencies::UnsupportedPortAction::Error;
Expand Down