-
Notifications
You must be signed in to change notification settings - Fork 440
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
Add psalm #510
Add psalm #510
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.
Hi @vesquen,
Thanks for your work on this task! I've added a couple of small remarks to the codebase.
You also need to link the task in doc/tasks.md
, otherwise the documentation won't be found by the users.
src/Task/Psalm.php
Outdated
'ignore_patterns' => [], | ||
'no_cache' => false, | ||
'report' => null, | ||
'threads' => 1, |
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.
Maybe it is better to set this to null
and let psalm decide what it uses by default?
ignore_patterns: [] | ||
no_cache: false | ||
report: ~ | ||
threads: 1 |
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 threads options is not described in the documentation
src/Task/Psalm.php
Outdated
$arguments->addOptionalArgument('--report=%s', $config['report']); | ||
$arguments->addOptionalArgument('--no-cache', $config['no_cache']); | ||
$arguments->addOptionalArgument('--threads=%d', $config['threads']); | ||
$arguments->addFiles($files); |
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.
During run
, the file list might get very big. Would it make sense to only add the files during pre-commit context and run on all files during run context?
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.
That does make sense, will fix!
Thanks for adding the docs, we'll merge it in with the next milestone release! |
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.
@vesquen Thanks!
It adds a task for Psalm
New Task Checklist:
run()
method readable?run()
method using the configuration correctly?