search

codeclimate / codeclimate-phpcodesniffer

Code Climate Engine for PHP Code Sniffer
MIT License
28 stars 23 forks source link

Use `include_paths` to determine files to analyze #22

Closed BlakeWilliams closed 8 years ago

BlakeWilliams commented 8 years ago

This removes the check for the inclusion of include_paths in favor of always using include_paths. This is because the use of exclude_paths for engines has been deprecated.

fhwang commented 8 years ago

Not that this should hold up review, but presumably this can't be merged until #21 is, right?

BlakeWilliams commented 8 years ago

If I can have my way I'll merge it in PR order.

This PR probably needs more discussion between @pbrisbin, @mrb, and I.

pbrisbin commented 8 years ago

@fhwang why do you think this is dependent on that? This PR intends to remove a dead path. So should be by-definition behavior neutral.

We need @mrb or @brynary to weigh in if this is safe to do yet.

My understanding is that new(ish) CLI and dot-com are updated to pass include-paths, so any engines that only support that approach will work fine.

There can be, however, older CLI images still in use in the wild and if they get a hold of this new phpcodesniffer engine image, things would likely break as the older CLI will pass in only exclude-paths.

Now, if that results in things blowing up (which I think it would), I'd be totally fine with it -- the first thing users typically do (or the first thing we'd suggest in a GH issue) is update. If, however, it results in bad (but not obviously broken) behavior, I'd want to be more careful.

fhwang commented 8 years ago

Yeah, actually I think I'm missing some context. It sounds like this is currently broken for people with an updated CLI and without any default extensions configured? I agree @pbrisbin that this should be behavior neutral if you have an up-to-date CLI.

BlakeWilliams commented 8 years ago

20 and #21 fix the engines odd or broken behavior. This PR is to remove checks on include_paths and only use include_paths since exclude_paths is deprecated.

jpignata commented 8 years ago

Closing due to staleness.