-
Notifications
You must be signed in to change notification settings - Fork 166
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
WP CLI Configuration File Check #597
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You've done a great job. Thanks for your effort in implementig tests too.
I'd just like to keep on discussing about cli_config_exists?
logic, which is not about code, but about the approach to be used to determine if wp-cli.yml
exists.
@pioneerskies Based on our conversations and some experimentation on a few edge cases, I've expanded the logic to use the following workflow:
I feel like this should reasonably account for most corner cases, as well as avoid any situations where the user's CWD incorrectly pulls in a WP CLI configuration from an installation they might not actually be referencing. Let me know if that makes sense to you. |
I'll review it ASAP, but this weekend I'll probably be off. Be faithful and patient, please 🙏 😄 |
No worries, man. Just trying to help where I can. Have a great weekend! |
I've been quite busy the last weeks. I'm catching up all these interesting discussions. You've done an amazing job 👏 Using |
Generally what I'm trying to do is to avoid "overmocking". There were too much stub we was relying on, but I opted to maintain only the one about invoking the `wp` command, so that we don't need to have the binary in the repo for running tests. I've also corercted the texts and reorganized the contexts with a different nesting. Now they look like this: ``` Wordmove::SqlAdapter::Wpcli #command having wp-cli.yml in local_path returns the right command as a string without wp-cli.yml in local_path but still reachable by wp-cli returns the right command as a string without any wp-cli configuration returns the right command with '--path' flag set to local_path ```
Maiorano pr 597 review
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
Approved! Thanks for the work, the patience and the share. I'll merge it really soon, together with another PR and will release a new version at the same time. |
@pioneerskies Thank you so much for your support and patience in all this. This is my first time even looking at or touching Ruby in my career, so I'm incredibly grateful you were willing to work with me on this and help me adhere to better Ruby practices. This is a regular tool we use in our company's WP workflows, so having a better understanding of what's going on under the hood will help all of us. Looking forward to the next tagged version release! |
@maiorano84 it was my pleasure. Knowing there are people and companies involved in using and contributing (with suggestions, code, acknowledged and complete bug reports) to Wordmove is a big present for any maintainer passionated about OSS. |
FYI @maiorano84 @nlemoine , I've updated documentation https://github.com/welaika/wordmove/wiki/movefile.yml-configurations-explained#global-options |
Fix bug in AdaptRemoteDb Undefined method `waring` was called on logger Fix wrong signature on method call in AdaptRemoteDb Warn when db adapt is skipper SetupContextForDb won't run unless necessary Update some log messages for better clarity Fix mu-plugins key in mu_plugins. Fixes #619 Please the rubocop Remove FTP support and cleanup as consequently Re-implement #597 in this branch Improve wpcli_search_replace_command with argument check Add yard documentation Fix bug in FilterAndSetupTasksToRun action Was getting key by string on a symbolized hash Migreate from Thor do dry-cli Improve UX a bit w/ better log messages Remove unused WordpressDirectory::HelperMethods module Re-enable --debug flag Update version: will stick to a pre-release Update scaffold and namespace for organizers WIP: re-implement FTP support WIP: re-implement FTP support Fix typo and reword a variable WIP: re-implement FTP support WIP: re-implement FTP support WIP: re-implement FTP support WIP: re-implement FTP support WIP: re-implement FTP support WIP: re-implement FTP support WIP: re-implement FTP support WIP: re-implement FTP support Update get_file_spec Update hook_spec Update rspec config Add adapt_local_db_spec Various cleanups, typos, rubocop disables Fix Wordmove::Movefile by removing - never triggered - wrong logic Update deps and please the (new) rubocop
This is a possible approach to handling issues #590 and #591 concerning how the WP CLI path option is handled when
wordpress_path
is pointed to a non-standard project root (ie: Bedrock)The idea here is that --path is only added when a wp-cli.yml file does not exist. In the case that one does exist, then WP CLI will fall back on the configuration there to allow the user to specify the path to Wordpress.
An extra test case has also been added to account for both scenarios.
I don't actually know Ruby, so I have this in draft status in case I'm doing anything that you don't agree with.
The one change here that might be debatable is using an array join to create the WP CLI command. I do this instead of standard string concatenation to handle whitespace a little more predictably and possibly improve readability.