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

Add associated args --new and --old #150

Closed
wants to merge 7,534 commits into from
Closed

Conversation

marksabbath
Copy link
Contributor

This PR introduces 2 new associated arguments to the search-replace command in an effort to resolve the bug reported on #132

With this approach, if users need to replace strings starting with more than one dash (i.e --mystring) they will be able to do that by using the new associated arguments: wp search-replace --old='--oldstring' --new='newstring'.

gitlost and others added 30 commits November 21, 2017 00:39
Update scaffolded README and GitHub configuration
Restrict some search-replace tests to wp_posts to avoid wp_options cl…
Add `--skip-tables=<tables>` argument to exclude tables when performing search-replace
Remove unnecessary `array_diff()`; skipping within loop is clearer
Convert search-replace subcommand help summaries to use third-person singular verbs.
@schlessera
Copy link
Member

We need to check whether both $old and $new were provided by the user, in any combination.

As you've probably noticed, we cannot check this based on the value of the arguments, as an empty string is a valid value (at least for $new), so we have to check based on the presence of the arguments/flags instead.

Also, we'll need functional tests to ensure the flags work as expected.

@marksabbath
Copy link
Contributor Author

We need to check whether both $old and $new were provided by the user, in any combination.

As you've probably noticed, we cannot check this based on the value of the arguments, as an empty string is a valid value (at least for $new), so we have to check based on the presence of the arguments/flags instead.
Great point @schlessera I'll work on that.

Also, we'll need functional tests to ensure the flags work as expected.
Yeah, I rebased with that purpose. I'll work on that on my next round :)

@marksabbath
Copy link
Contributor Author

Hey @schlessera would you mind taking another look at this PR?

@@ -95,7 +96,7 @@ jobs:
uses: repo-sync/pull-request@v2
with:
source_branch: regenerate-readme
destination_branch: master
destination_branch: ${{ github.event.repository.default_branch }}
github_token: ${{ secrets.GITHUB_TOKEN }}
pr_title: Regenerate README file
pr_body: "**This is an automated pull-request**\n\nRefreshes the `README.md` file with the latest changes to the docblocks in the source code."
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to update your branch so the changes to .github/* don't appear in the diff?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean rebase it? I did that before pushing, but will do that again.

@@ -47,10 +47,16 @@ class Search_Replace_Command extends WP_CLI_Command {
*
* ## OPTIONS
*
* <old>
* [<old>]
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 love that we're changing the behavior of the positional arguments to fix this very small edge case.

Might it be possible to handle this in wp-cli/wp-cli somehow? Better handling of positional arguments that look like associative arguments?

Alternatively, is there some other workaround that could be employed for this edge case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's been quite a long time since I did the tests, but I do remember that I couldn't find a way to make that work properly without using this approach.

Maybe a change under wp-cli/wp-cli making it aware of which are the accepted associative args and use it as positional if it does not recognize it?

@danielbachhuber
Copy link
Member

Proceeding with wp-cli/wp-cli#5594 for this repository. I've captured this PR to https://gist.github.com/danielbachhuber/6c398217cff94e8a23a94b6b23aca3aa in case this PR is auto-closed or broken in some way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.