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 bootstrap file option #783

Merged
merged 1 commit into from
Nov 27, 2015
Merged

Add bootstrap file option #783

merged 1 commit into from
Nov 27, 2015

Conversation

johnmaguire
Copy link
Contributor

This adds a longopt for specifying bootstrap files. An example of why this might be useful:

<?php

PHP_CodeSniffer::$allowedTypes = array_merge(PHP_CodeSniffer::$allowedTypes, ['int', 'bool']);

You can set a bootstrap file in ruleset.xml using the <arg> element. Previously we were modifying $allowedTypes in our custom sniff files, but there's a race condition unless you add this to every single sniff file you have (and you end up with lots of dupes unless you run array_unique each time.)

@gsherwood
Copy link
Member

This is an interesting idea, but I feel like it is something that should be included later on in the process. Maybe after all command line arguments have been processed and the run is about to start.

Possibly right here: https://github.com/squizlabs/PHP_CodeSniffer/blob/master/CodeSniffer/CLI.php#L782

And in the 3.0 branch, probably at the top of the main run() method: https://github.com/squizlabs/PHP_CodeSniffer/blob/3.0/src/Runner.php#L245

Does that still let you do what you need?

@johnmaguire
Copy link
Contributor Author

@gsherwood I believe so. I figured it probably wasn't the right place for it, but didn't know where was. The main thing is the bootstrap needs to be run before the sniffs are. (In the example I gave, my use case, we want to make sure $allowedTypes is updated before any sniffs' checks are run.)

Would you like a separate pull request for each branch?

@gsherwood
Copy link
Member

@johnmaguire No need for two PRs. 3.0 isn't stable yet so it may not work for you. It's also completely different, so I'll port it over based on how I've been converting things in there.

If you think the idea of storing the bootstrap location in the $_cliArgs member var and then including the file around line 782 is a good one, just modify the PR for that and I'll get it merged in.

@johnmaguire
Copy link
Contributor Author

@gsherwood I hope I understood correctly! I was able to base my code roughly off the sniffs option above as well.

@gsherwood
Copy link
Member

I hope I understood correctly!

Exactly what I was thinking. I can't give it a good test right now, but will test and merge tomorrow. Thanks a lot.

@gsherwood
Copy link
Member

Thanks a lot for the patch. There 3.0 commit is here: 88c587f

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.

2 participants