-
Notifications
You must be signed in to change notification settings - Fork 430
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
Improved exotic paths #644
Conversation
$this->gitDirLocator = $gitDirLocator; | ||
} | ||
|
||
public function locate(?string $cliConfigFile): GuessedPaths |
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.
@Landerstraeten : moved all path guessing logic to this locator.
Can you go through it to see if it makes sense to you?
This one is constructed early in the cli boot code and will be used to tell GrumPHP where to look for stuff. (git, bins, configs, ...)
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.
Only checked it high level, but it looked alright to me.
Just a few minor questions.
@@ -3,9 +3,15 @@ services: | |||
class: GrumPHP\Event\Subscriber\StashUnstagedChangesSubscriber | |||
arguments: | |||
- '@config' | |||
- '@git.repository' | |||
- '@GrumPHP\Git\GitRepository' | |||
- '@grumphp.io' |
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.
Any plans to use fqcn's as service names everywhere?
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.
Currently no plans for that. it's something that is happening gradually while we are improving stuff.
We could create a PR to fix them all, but it might break extensions.
); | ||
} | ||
|
||
return trim($process->getOutput()); |
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.
this is always correct? even when multiple executables exist (weird situation i know)
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.
It should be. We were using it previously as well and I don't know of a case where it doesnt do what it promises.
(Actually: when you run this in a php script in a subdir of the git root during pre-commit, it crashes. This is a git bug which is very vague, so I fixed it by adding the GRUMPHP_GIT_WORKING_DIR
in the local git commit.)
private $repositoryOptions; | ||
|
||
/** | ||
* @var ?Repository |
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.
@veewee, what's the best way to typehit: null|Repository
or ?Repository
?
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.
null|Repository
works best in most IDEs I guess ...
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.
Very, very nice @veewee!
This PR aims to fix some well known issues with exotic paths.
Previous PR (#643) introduced an issue when using conventions.
This PR will also try to fix issues when composer, git and grumphp config files are in separate directories.
This PR:
bin_dir
: it is loaded from composer config instead.git_dir
: it tries to locate the git dir on every run.Manually tested: vagrant, vagrant preset, composer-bin-plugin, docker
You can test it as well:
Note:
bin_dir and git_dir are removed from config. But it doesn't matter if it's still in there