Skip to content

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

Merged
danielbachhuber merged 12 commits intowp-cli:mainfrom
sejas:update/confirm-dist-file-exists-before-overwrite
Dec 6, 2023
Merged

Ask user for confirmation before overwriting if file already exists#85
danielbachhuber merged 12 commits intowp-cli:mainfrom
sejas:update/confirm-dist-file-exists-before-overwrite

Conversation

@sejas
Copy link
Contributor

@sejas sejas commented Nov 10, 2023

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
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

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

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 Ask user for confirmation before overwriting if file already exists Dec 6, 2023
@danielbachhuber danielbachhuber added the command:dist-archive Related to 'dist-archive' command label 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
@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 Related to 'dist-archive' command

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants