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 logic to remove the project directory after 'cgr remove' #21

Closed
wants to merge 4 commits into from
Closed

Conversation

uberhacker
Copy link
Contributor

No description provided.

@coveralls
Copy link

coveralls commented Jul 3, 2017

Coverage Status

Coverage decreased (-2.9%) to 88.8% when pulling f8e622b on uberhacker:master into 2211ff2 on consolidation:master.

* prior to completing the process of iterating through all the projects.
*/
list(, $workingDir, $composerCommand, $orgProject) = explode(' ', $command->getCommandString());
if ($composerCommand == "'remove'") {
Copy link
Member

Choose a reason for hiding this comment

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

Could this be done in some other location, so that we do not need to decompose and parse the command string?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question. It would have to been done after the cgr remove command has executed. Do you have a suggested location?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suppose you could add a public method to the CommandToExec class that returns a parsed array of the command arguments.

Copy link
Member

Choose a reason for hiding this comment

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

  1. Change runCommandList() so that it will run either function pointers or exec external tools.
  2. Define a removeCommand() method, so that will be called instead of generalCommand for remove commands.
  3. Make removeCommand() call through to generalCommand() and also return a function pointer to the extra cleanup you want to do.

Doing 1 implies a new base class for things to execute.

Copy link
Contributor Author

@uberhacker uberhacker Jul 3, 2017

Choose a reason for hiding this comment

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

Sounds complicated. :) I'm not so sure calling through generalCommand() will produce the intended results unless you mean encapsulating the entire run process? Remember, we can only remove the directory after the composer remove command is executed. What do you think of just bypassing the composer remove command altogether and just remove the directory?

Copy link
Member

Choose a reason for hiding this comment

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

Completely replacing the composer remove command would be a fine option.

$projectSpecificArgs = array("--working-dir=$installLocation", $composerCommand);
if (!empty($projectWithVersion)) {
$projectSpecificArgs[] = $projectWithVersion;
if ($composerCommand == 'remove') {
Copy link
Member

Choose a reason for hiding this comment

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

Nice, but you don't need to add special-checking to buildGlobalCommand(). If you define a method removeCommand(), it will be called instead. You can then build and return whatever sort of CommandToExec() you'd like.

@coveralls
Copy link

coveralls commented Jul 3, 2017

Coverage Status

Coverage increased (+0.1%) to 91.803% when pulling 7be9680 on uberhacker:master into 2211ff2 on consolidation:master.

@greg-1-anderson greg-1-anderson mentioned this pull request Jul 4, 2017
@uberhacker
Copy link
Contributor Author

Closing since #22 is a better way.

@uberhacker uberhacker closed this Jul 4, 2017
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.

None yet

3 participants