Skip to content
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 whitelist_patterns option to Phpcs task #160

Merged
merged 9 commits into from
Nov 20, 2016
Merged

Conversation

mgeoffray
Copy link
Contributor

@mgeoffray mgeoffray commented Jul 6, 2016

Q A
Branch master for features and deprecations
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes/no
Documented? yes
Fixed tickets #158

This PR add a new whitelist_patternsoption to Phpcs task. It will be used to filter files to be validated.

Example for a Symfony, validate PHP files only in src/ directory

phpcs:
    standard: "./vendor/leaphub/phpcs-symfony2-standard/leaphub/phpcs/Symfony2/"
    show_warnings: true
    whitelist_patterns: ["^src/(.*)"]
    triggered_by: [php]


/** @var string|null $whitelistPathPattern */
$whitelistPathPattern = $this->getWhitelistPathPattern($config);
if (!$whitelistPathPattern) {
Copy link
Contributor

@veewee veewee Jul 8, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't both the whitelist as the extensions filter be applied?
This can be done in 2 methods instead of adding the extensions with the getWhitelistPathPattern method.

@veewee
Copy link
Contributor

veewee commented Jul 8, 2016

Thanks for the PR. Could you add some tests for this feature?

@mgeoffray
Copy link
Contributor Author

mgeoffray commented Jul 8, 2016

Hi @veewee,
no problem I will take a look this weekend.

Is the option name whitelist_path_pattern is fine for you? I was not very inspired at the moment.

Thank you for your feedback.

@veewee
Copy link
Contributor

veewee commented Jul 8, 2016

It looks good. Alternatives are include_patterns, whitelist_pattern, ...
Maybe it should also work like the ignore patterns so that you can provide multiple paths that are whitelisted?

@mgeoffray
Copy link
Contributor Author

I change parameter name to whitelist_patterns and make it usable as a regex array

@veewee
Copy link
Contributor

veewee commented Jul 12, 2016

It looks pretty good but I would still like to get rid of the escapePatternDirectorySeparator() method.
I also think its better to run the extensions() filter separate from the paths() filter. This way the extension filter won't break when someone enters a bad regex.
Can you also resolve the conflicts and fix the Continious Integration?

@mgeoffray
Copy link
Contributor Author

Hi @veewee sorry for the late answer, I will take a look this week at your return.
Thanks

@veewee
Copy link
Contributor

veewee commented Aug 28, 2016

@mgeoffray Are you planning to finish this PR any time soon? Thanks!

@mgeoffray
Copy link
Contributor Author

@veewee Yes sorry too much work beside, I prioritized it to correct your return quickly ;)

@veewee
Copy link
Contributor

veewee commented Sep 1, 2016

No problem! Thanks :)

@j3rrey
Copy link
Contributor

j3rrey commented Sep 22, 2016

Is anyone taking care of this ?

@veewee
Copy link
Contributor

veewee commented Sep 23, 2016

@jeremy-bruns, @mgeoffray is taking care of this. Currently he is busy so this new feature is on a hold. Feel free to contact him, maybe you can take care of this for him.

@mgeoffray
Copy link
Contributor Author

@jeremy-bruns Hi, yes I plan to watch it from the beginning of next week on Monday or Tuesday maximum. I will keep you in touch!

@mgeoffray
Copy link
Contributor Author

I worked on it this week I think I can deliver the fix this weekend ;)

@mgeoffray mgeoffray changed the title Add whitelist_path_pattern option to Phpcs task Add whitelist_patterns option to Phpcs task Nov 16, 2016
@mgeoffray
Copy link
Contributor Author

Hi @veewee,
someone is looking for your return today so I will keep you in touch quickly.
Again sorry for processing delay

@mgeoffray
Copy link
Contributor Author

Hi @veewee,
we have just finished to process your return let me know if that is all good for you ! ;)

Copy link
Contributor

@veewee veewee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your work. I've added some litlte remarks.

*
* @return FilesCollection
*/
public function paths(array $patterns)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code is almost the same as the regular path() method. Can you let the path() method use the paths() method instead of duplicating the code?

There should also be a spec test for this new feature.
Note that we are moving away from the old array() syntax and are using the new short array syntax instead. Can you replace that one?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@veewee Yes you are right I refactor the path() function to call paths()
I forgot this one array, I have changed it.
What do you mean spec test, a new function testing it or just add the new line in \spec\GrumPHP\Task\PhpcsSpec::it_should_have_configurable_options ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, the configurable options spec from the task should also contain the new option. The last commit is good.
I was talking about the GrumPHP/Collection/FilesCollectionSpec that should contain a new test for the new paths method. Can you add this one?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @veewee,
I have just added the function for paths() Spec test

@veewee veewee added this to the Version 0.10.0 milestone Nov 17, 2016
@veewee
Copy link
Contributor

veewee commented Nov 19, 2016

It looks good, thanks!
I'll merge it in somewhere next week when the nightly build is fixed. This has nothing to do with this code, so you don't have to do any additional work on this PR.

@mgeoffray
Copy link
Contributor Author

@veewee OK great ! Thanks :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants