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

Ask user for confirmation before overwriting if file already exists #85

Merged
merged 12 commits into from
Dec 6, 2023
Merged

Ask user for confirmation before overwriting if file already exists #85

merged 12 commits into from
Dec 6, 2023

Conversation

sejas
Copy link
Contributor

@sejas sejas commented Nov 10, 2023 •
edited
Loading

Description

I'm checking if archive_absolute_filepath exists, then we display a warning and ask for confirmation to overwrite.
I also added a new argument --yes to auto confirm and overwrite anyway. In this case we still showing the warning.

Testing instructions

I added a new test:Ask for confirmation if archive file exists.

Here are the steps for manual testing:

Given a "pluginPath":

  1. Run vendor/bin/wp dist-archive pluginPath
  2. Observe it display a success message: Success: Created ...zip
  3. Run the same command vendor/bin/wp dist-archive pluginPath
  4. Observe the cli displays a warning and asks for confirmation. ``
  5. type s
  6. Observe the cli exists with the following message: ``
  7. Run the same command vendor/bin/wp dist-archive pluginPath
  8. type r
  9. Observe it display a success message: Success: Created ...zip
  10. Run the same command prepending a string confirmation echo "r" | vendor/bin/wp dist-archive pluginPath
    11.Observe it display a warning and a success message without the need to prompting the user: Success: Created ...zip without asking for confirmation.

Example command execution

Normal execution

❯ vendor/bin/wp dist-archive /Users/macbookpro/demo-wp-now/community-themes/atlas
Success: Created atlas.0.0.1.zip

Skipping

❯ vendor/bin/wp dist-archive /Users/macbookpro/demo-wp-now/community-themes/atlas
Warning: File already exists
/Users/macbookpro/demo-wp-now/community-themes/atlas.0.0.1.zip
Do you want to skip or replace it with a new archive? [s/r]: s
Skipping

Archive generation skipped.

Replacing

❯ vendor/bin/wp dist-archive /Users/macbookpro/demo-wp-now/community-themes/atlas
Warning: File already exists
/Users/macbookpro/demo-wp-now/community-themes/atlas.0.0.1.zip
Do you want to skip or replace it with a new archive? [s/r]: r
Replacing

Success: Created atlas.0.0.1.zip

Auto Replacing

❯ echo "r" | vendor/bin/wp dist-archive /Users/macbookpro/demo-wp-now/community-themes/atlas
Warning: File already exists
/Users/macbookpro/demo-wp-now/community-themes/atlas.0.0.1.zip
Do you want to skip or replace it with a new archive? [s/r]: Replacing

Success: Created atlas.0.0.1.zip
@sejas sejas requested a review from a team as a code owner November 10, 2023 18:38
danielbachhuber
danielbachhuber reviewed Nov 10, 2023
View reviewed changes
src/Dist_Archive_Command.php
if ( file_exists( $archive_absolute_filepath ) ) {
WP_CLI::warning( "The file '{$archive_absolute_filepath}' already exists." );
WP_CLI::confirm( 'Do you want to overwrite it?', $assoc_args );
}
Copy link
Member

Choose a reason for hiding this comment

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

I'm not quite sure WP_CLI::confirm() is right.

Here's the behavior for wp scaffold package-readme:

$ wp scaffold package-readme ./
Warning: File already exists
.//README.md
Skip this file, or replace it with scaffolding?[s/r]:

Here's the behavior for wp core download:

$ wp core download
Error: WordPress files seem to already be present here.
$ wp core download --force
Downloading WordPress 6.4.1 (en_US)...
md5 hash verified: 5f9044e6b3f78f1bbdf85fed0244f778
Success: WordPress downloaded.

@BrianHenryIE What prior art do you think we should follow?
Copy link
Contributor Author

@sejas sejas Nov 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@danielbachhuber , I refactored it here to match the scaffolding behaviour:
3ef862d

I added a test and updated the PR description with the Example command execution.
Copy link
Member

Choose a reason for hiding this comment

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

@sejas Nice job on the tests!

I think I like this output but I'm not sure I love it:

$  wp dist-archive ./
Warning: File already exists
/Users/danielbachhuber/wordpress-plugins/one-time-login.zip
Do you want to skip or replace it with a new archive? [s/r]:

@BrianHenryIE @wp-cli/committers Any opinions on how it could be improved?
Copy link
Contributor Author

@sejas sejas Dec 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@danielbachhuber , what changes would you make to love the output?
Copy link
Member

Choose a reason for hiding this comment

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

I don't think I have a better suggestion than the current form, which seems fine
Copy link
Member

Choose a reason for hiding this comment

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

I changed 'File already exists' to 'Archive file already exists': 58105cf

Now I'm happy 😁
@danielbachhuber danielbachhuber changed the title Dist Archive: ask user confirmation if file exists before overwriting it Dec 6, 2023
@danielbachhuber danielbachhuber added this to the 3.0.0 milestone Dec 6, 2023
@danielbachhuber danielbachhuber merged commit 926c9eb into wp-cli:main Dec 6, 2023
23 checks passed
@sejas sejas deleted the update/confirm-dist-file-exists-before-overwrite branch January 29, 2024 11:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
command:dist-archive
3 participants
@sejas @danielbachhuber @swissspidy