-
-
Notifications
You must be signed in to change notification settings - Fork 46
Check: adds an option to specifiy an autoloader file to be included #489
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
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #489 +/- ##
============================================
+ Coverage 95.82% 95.89% +0.06%
- Complexity 657 665 +8
============================================
Files 76 76
Lines 1798 1827 +29
============================================
+ Hits 1723 1752 +29
Misses 75 75 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
We need to add this option to the readme... maybe in the phar section? |
src/CLI/Command/Check.php
Outdated
/** | ||
* @psalm-suppress UnresolvableInclude | ||
*/ | ||
protected function requireAutoload(?string $filePath, OutputInterface $output): void |
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.
Why not invert params to have the nullable value at the end?
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.
Yup, I guess so
src/CLI/Command/Check.php
Outdated
return $verbose ? new DebugProgress($output) : new ProgressBarProgress($output); | ||
} | ||
|
||
protected function createBaseline(bool $skipBaseline, ?string $baselineFilePath, OutputInterface $output): Baseline |
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.
same as above.
|
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.
LGTM 👍
Context: as we are considering the possibility to use functions like
is_a
in the rules, we need a way to deal with the autoload of the classes phparkitect is analizyngWe have two scenarios based on how phparkitect is installed
as a project dependency via composer: this is the simplest case since, since phparkitect shares the autoload with the project it analyzes, no additional autoloader is needed ✅
as a phar: here phparkitect autoload is embedded in the phar itself, so we need a way to autoload the project's classes
In the second scenario, we should consider the phar could be in different locations:
/usr/local/bin/phparkitect
bin
dirso trying to guess where the project autoload could be can be challenging. Possible solutions to tackle that:
Solution no 2. would be the most consistent with the current approach as the ClassDescription object would contain all the info needed to apply a rule and no autoload will be triggered. This is also the most complex to implement properly and it may need rethinking the architecture which now assumes we parse a file -> create one or more ClassDescription objects -> apply rules on them.
Solution no.3 is kinda common on other tools (rector, psalm, phpstan) so it should not be a big surprise for the users. It is also the easiest to implement. Is there any chance of getting conflicts between phparkitect's classes and the project classes? No, because the tool we use to package the phar uses php-scoper to avoid this kind of problems
Choosen solution: while it could be nice to do spike in order to understand the effort of implementing 2. it properly here I went for solution no 3.: adding the capability to specify the path of an extra autoloader file via command line option.
this should allow what is asked in #401