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

Adds a new command that attempts to clean up installed .php-manager files #6

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

marcusedwardhaslam
Copy link
Owner

No description provided.

src/CLI/Application/App.php Outdated Show resolved Hide resolved
src/CLI/Application/App.php Outdated Show resolved Hide resolved
src/Lib/PHPManagerConfiguration.php Outdated Show resolved Hide resolved
src/Lib/PHPManagerConfiguration.php Outdated Show resolved Hide resolved
src/Lib/PHPManagerConfiguration.php Outdated Show resolved Hide resolved

final readonly class PHPManagerConfiguration
{
function __construct(public string $phpManagerDirectory = '/.php-manager', public string $distDirectory = '/dist')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Proposal: istead of distDirectory=dist, you could have distRelativeDirectory=dist, which would make it clearer that it is relative to phpManagerDirectory. Then we could construct distDirectory=$distDirectory/$distRelativeDirectory. Likewise we could have a phpDir, buildDir and composerDir constructed the same way. Although at this point might be worth extracting another feat PR to introduce the configuration.

Entirely personal, when there is this kind of logic, I tend to prefer to put it in another factory rather than the constructor, to keep the ability to create the object without any extra processing (either for perf reasons or for convenience in tests), e.g.:

readonly class Configuration {
  function __construct(
    public string $phpManagerDir,
    public string $distDirectory,
  ) {}

  static function create(
    string $phpManagerDir,
    string $distRelativeDir,
  ) {
    return new self(
      $phpManagerDir,
      $phpManagerDir.'/'.$distRelativeDir,
    );
  }
}

Copy link
Owner Author

Choose a reason for hiding this comment

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

Dist is currently outside of the php manager dir. It does make sense to move it inside, then we can fix this... But it'll require changing the way the installer works, I'll leave that for another PR.

$io->writeln("Removing PHP, Composer and all other project dependencies.");

$cwd = getcwd();
$this->filesystem->remove([$cwd . $this->config->phpManagerDirectory, $cwd . $this->config->distDirectory]);
Copy link
Collaborator

Choose a reason for hiding this comment

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

since distDirectory is contained within phpManagerDirectory, we could just remove the latter no? Or did I get the order of the dirs wrong

Copy link
Owner Author

Choose a reason for hiding this comment

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

See above

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.

2 participants