Reuse existing PR when available. #2
Conversation
If clu is run at a time when there is already an open pull request with Composer updates that have not yet been merged, then the existing branch will be updated. This prevents the accumulation of pull requests should the maintainer be slow about merging updates. Also, refactored to allow calling clu without any parameters, to operate on an existing local working copy of a project at the current working directory. This is handy for ad-hoc use, and also for use in a CI server in circumstances where the site to be updated has already been checked out by the CI server.
There was a problem hiding this comment.
Thanks for the pull request, @greg-1-anderson. Left a few comments to start.
| if ( ! $status ) { | ||
| $repo_url = $first_line_of_output; | ||
| } | ||
| } |
There was a problem hiding this comment.
Given the rest of the project is already using tabs, could we use tabs instead of spaces for indentation?
There was a problem hiding this comment.
Didn't notice that! I will fix it up.
src/Runner.php
Outdated
| @@ -136,6 +139,12 @@ public function start() { | |||
| Logger::error( 'Failed to push changes to origin.' ); | |||
| } | |||
|
|
|||
| if ( $existing_PR_branch ) { | |||
| // TODO: Add comment to existing PR with $message | |||
There was a problem hiding this comment.
Do you want to resolve this TODO?
There was a problem hiding this comment.
Unfortunately, hub does not have a 'comment' command. I started a pull request (mislav/hub#1465), but there has been no feedback, so I'm not sure that it's wanted. We could use the curl API.
src/Runner.php
Outdated
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
Any ideas on how we could get some basic unit or integration tests going for this?
There was a problem hiding this comment.
Functional tests for the basic functionality (pre this PR), it would be pretty easy. We could use composer create-project to clone a project that has out-of-date components, and then run clu on it. The Terminus Build Tools plugin uses similar techniques in its tests.
Functional tests for re-using a PR would be more difficult. Maybe we could cheat a bit to do this step. Create a PR per the above with clu; then, edit the composer.json to add a component that was not previously included in the system-under-test. At that point, running clu again will behave the same way that it would when there was an update. So, while we would not be testing the same use-case, we would be exercising the code in the same way, and I think that would be sufficient. Creating a mock packagist.org would be too much trouble, and mocking the API calls with something like VCR would also be more work, and less valuable.
I don't think that unit tests for this project would be particularly valuable.
Maybe we could tackle testing as a follow-on task? I have limited time prior to DrupalCon Vienna, but would be happy to help out with this on my return.
|
Fixed up indentation. To ensure that a project maintains a consistent code style, it is helpful to enforce style rules during the test. I added phpcs + phpcbf rules to enforce TABS instead of SPACES. I did not enforce any other PHP coding standards, though, as the conventions you are following here do not seem to match any of the standards that come bundled with phpcs. I would suggest adopting a common standard, e.g. PSR-2. I implemented the commit comment TODO. I would prefer to put off adding tests until later, per my previous comment. |
|
@greg-1-anderson Thanks again. Tagged a v0.2.0 release for this. |
If clu is run at a time when there is already an open pull request with Composer updates that have
not yet been merged, then the existing branch will be updated. This prevents the accumulation of
pull requests should the maintainer be slow about merging updates.
Also, refactored to allow calling clu without any parameters, to operate on an existing local working
copy of a project at the current working directory. This is handy for ad-hoc use, and also for use
in a CI server in circumstances where the site to be updated has already been checked out by the CI
server.