-
Notifications
You must be signed in to change notification settings - Fork 0
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
base: main
Are you sure you want to change the base?
Conversation
src/Lib/PHPManagerConfiguration.php
Outdated
|
||
final readonly class PHPManagerConfiguration | ||
{ | ||
function __construct(public string $phpManagerDirectory = '/.php-manager', public string $distDirectory = '/dist') |
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.
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,
);
}
}
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.
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]); |
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.
since distDirectory
is contained within phpManagerDirectory
, we could just remove the latter no? Or did I get the order of the dirs wrong
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.
See above
Co-authored-by: Théo FIDRY <[email protected]>
No description provided.