-
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
Added Php-cs-fixer support #14
Conversation
Can you update the php version requirement in composer.json so that travis can do its thing? @veewee I think bumping the php version to 5.3.6 should be ok if we decide to accept this PR? |
Please, check, i updated PHP version and fixed codestyle errors. |
Hello @klipach It sounds 'scary' to alter files during a commit. You won't have control about those files anymore. It's a good thing to make the Could you also add a configurable These are some general remarks about the future structure of GrumPHP. These might not be fixed in this issue, but I would like to have some input on this: Another remark I have concerning this auto_fix variable is the priority of the tasks. By changing the files you might break some existing tasks. Maybe it's a good idea to add a task priority per task configuration? For example: run the php-cs-fixer task first, next all other tests. At the moment the Task::run command is rather bug. We might introduce a 'Formatter' interface to display success / warning / error messages. Not sure how it will look for the moment, but it will remove the formatting stuff from the Task::run command. It seems like a good idea to me. Another thing that ennoys me in the Task::run commands is this part: if ($config['whatever']) {
$processBuilder->add();
} Maybe it's a good idea to create a wrapper around the Symfony ProcessBuilder with following additional methods: // Add an optional configuration option. This will not add anything if the config is empty
$wrapper->addOptionalAttribute('--config=%s', $config['config']);
// Add a required configuration options. Throws exception when the config is empty
$wrapper->addRequiredAttribute('--config=%s', $config['config']);
// Clone the processbuilder wrapper
$wrapper->clone() |
Hi @klipach, Me and @aderuwe discussed this issue. It seems like a good idea to drop the The All other remarks are placed in new issues: #16, #18 and #19. Looking forward to hear your feedback. |
Thank you for comments, i agree that |
There is very cool tool PHP-CS-Fixer it allows to fix codestyle errors automatically.