Skip to content

Support project's local phpcs installation#666

Merged
w0rp merged 5 commits into
dense-analysis:masterfrom
Firehed:phpcs
Jun 21, 2017
Merged

Support project's local phpcs installation#666
w0rp merged 5 commits into
dense-analysis:masterfrom
Firehed:phpcs

Conversation

@Firehed

@Firehed Firehed commented Jun 19, 2017

Copy link
Copy Markdown
Contributor

This change allows use of the PHPCS linter when installed as a project's dev dependency, instead of only a global install. It firsts checks for vendor/bin/phpcs as the executable path before falling back to the previous behavior (phpcs anywhere on the $PATH). This is similar to how many Node-based linters work, but with the paths from PHP's Composer.

@w0rp w0rp left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Good idea.

Could you cover this with some tests? There are a few tests which have some blank files in some directories inside of the test directory and set the filename for the buffer so it picks up on local executables. You might have to run chmod ug+x for the dummy executable files.

Comment thread ale_linters/php/phpcs.vim Outdated
\ : ''

return 'phpcs -s --report=emacs --stdin-path=%s ' . l:standard_option
return ale_linters#php#phpcs#GetExecutable(a:buffer)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You should ale#Escape the executable here, as the path might contain spaces, etc.

Comment thread ale_linters/php/phpcs.vim Outdated
\ a:buffer,
\ 'vendor/bin/phpcs',
\ 'phpcs'
\)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could you make the global executable configurable and add a use_global option while you're at it, similar to eslint and others?

@Firehed

Firehed commented Jun 20, 2017

Copy link
Copy Markdown
Contributor Author

Thanks for the feedback! It's my first time writing any vimscript (outside of vimrc); I'll try to address your concerns and update the pull shortly.

@w0rp w0rp left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nicely done. Looks good to me. 👍 I might see if new can be replaced with file in the tests.

Comment thread ale_linters/php/phpcs.vim
call ale#Set('php_phpcs_use_global', 0)

function! ale_linters#php#phpcs#GetExecutable(buffer) abort
return ale#node#FindExecutable(a:buffer, 'php_phpcs', [

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You fell into the "pit of success" here. I might rename this because it turns out it works for more than just node programs.

@w0rp w0rp merged commit ab534c2 into dense-analysis:master Jun 21, 2017
@partounian

Copy link
Copy Markdown

I'm using root/sage (8.5.1 latest production release) which is a WP theme. This reason this is important is because I also have template overrides. The template overrides are done with WP Coding Standards that way diffs can be checked against the originals when needed, and the rest of the theme follows the Roots guidelines (PSR-2 with slight modifications). I have PHPCS scripts for both of them.

What is the best way to handle linting for this situation?

(btw just switched to ale from neomake and loving the performance boost so far)

@Firehed

Firehed commented Jul 18, 2017

Copy link
Copy Markdown
Contributor Author

PHPCS should pick up a phpcs.xml in the project root automatically (or, at least, it does for me). However there's a g:ale_php_phpcs_standard variable that you can set in your vimrc that will let you explicitly pass a --standard=<that value> flag to the command.

@partounian

Copy link
Copy Markdown

Yes but what if the project has two different set of styles? For example the main dir all subdirectories follow a variant of PSR-2 except for files in /templates and /woocommerce.

@Firehed

Firehed commented Jul 18, 2017

Copy link
Copy Markdown
Contributor Author

Based on this page, it looks like PHPCS may pick up on directory-relative ruleset files. Failing that, there appears to be at least some support for specifying included/excluded files to lint in the phpcs.xml file, although it's not clear to me from their docs if you could set up two configurations "next to each other" in that way.

You might be able to rig something up with an ftdetect script; something like au BufNewFile,BufRead */templates/*.php set g:ale_php_phpcs_standard=path/to/rules.xml. Definitely not an ideal solution, but I think PHPCS really isn't designed to allow for multiple standards within the same project.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants