-
Notifications
You must be signed in to change notification settings - Fork 436
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
Feature/phan #336
Feature/phan #336
Conversation
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.
Thanks for the PR! I've added some small changes;
.phan/config.php
Outdated
@@ -0,0 +1,3 @@ | |||
<?php |
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 this package is not running han by default, it is ok to remove this file?
README.md
Outdated
- kahlan/kahlan: ~3 | ||
- nikic/php-parser : ~2.1 | ||
- kahlan/kahlan : ~3 | ||
- etsy/phan : dev-master@dev |
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.
Can you use one of the stable versions instead?
grumphp.yml.dist
Outdated
@@ -7,6 +7,7 @@ parameters: | |||
ignore_patterns: | |||
- "spec/*Spec.php" | |||
- "test/*.php" | |||
- ".phan/*.php" |
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 one can be removed when you remove the config file that isn't used.
spec/GrumPHP/Task/PhanSpec.php
Outdated
{ | ||
$options = $this->getConfigurableOptions(); | ||
$options->shouldBeAnInstanceOf(OptionsResolver::class); | ||
$options->getDefinedOptions()->shouldContain('exclude'); |
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.
The config_file setting is missing
spec/GrumPHP/Task/PhanSpec.php
Outdated
@@ -0,0 +1,108 @@ | |||
<?php |
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.
There are 2 spec files in this PR. Can you use the PSR-4 version?
src/Task/Phan.php
Outdated
|
||
$arguments = $this->processBuilder->createArgumentsForCommand('phan'); | ||
|
||
$arguments->addOptionalArgumentWithSeparatedValue('-k', $config['config_file']); |
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.
Can you use the long argument names? That makes debugging a bit easier in the future :)
composer.json
Outdated
@@ -52,7 +52,8 @@ | |||
"sebastian/phpcpd": "Lets GrumPHP find duplicated code.", | |||
"sensiolabs/security-checker": "Lets GrumPHP be sure that there are no known security issues.", | |||
"squizlabs/php_codesniffer": "Lets GrumPHP sniff on your code.", | |||
"sstalle/php7cc": "Lets GrumPHP check PHP 5.3 - 5.6 code compatibility with PHP 7." | |||
"sstalle/php7cc": "Lets GrumPHP check PHP 5.3 - 5.6 code compatibility with PHP 7.", | |||
"etsy/phan": "Lets GrumPHP unleash a static analyzer on your code" |
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 list is alphabetically. Can you change the order?
doc/tasks/phan.md
Outdated
parameters: | ||
tasks: | ||
phan: | ||
config_file: ~ |
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.
Can you use the correct default values here?
src/Task/Phan.php
Outdated
|
||
$arguments = $this->processBuilder->createArgumentsForCommand('phan'); | ||
|
||
$arguments->addOptionalArgumentWithSeparatedValue('-config-file', $config['config_file']); |
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.
The documentation states that the long parameters are double dashed. This probably won't work with single dashes.
Thanks for your work! |
Added new task Phan
New Task Checklist:
run()
method readable?run()
method using the configuration correctly?