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 Composer scripts for code sniffing and autofixing #18

Closed
wants to merge 1 commit into from

Conversation

oliverklee
Copy link
Collaborator

@oliverklee oliverklee commented May 2, 2021

There now are the following new Composer scripts:

  • ci: run all CI-related Composer scripts
  • ci:static run the static code analysis
  • phpcs run PHP_CodeSniffer
  • fix fix the autofixable PHP code warnings
  • phpcbf fix the autofixable PHP_CodeSniffer warnings

For starters, we have a sniff that checks that all files are in
PHP strict mode. (The CI builds do not run this check as the files
are not strict yet at this point.)

The PHP_CodeSniffer autofixer cannot make files strict, though
(as this is a non-autofixable code sniff). It will be able to
autofix other things we'll add later, though.

@oliverklee
Copy link
Collaborator Author

Some background on PHIVE:

PHIVE is a package manager for PHP development tools that installs the tools as stand-alone, GPG-signed PHARs (i.e., a PHAR contains the corresponding tool with all its dependencies). This helps avoiding dependency hell - both inter-tools as well as between a tool and the project in which it is used. (Dependency hell is e.g., when package A requires package X in version < 3, while package B requires package X in version >= 3.) (This was a real problem in some of my projects.)

In addition, this allows tools like PHPStan to be used in their current version (which requires PHP >= 7.1) without breaking Composer-installability of the project in lower PHP versions. (This also is a real problem in some of my projects.)

PHIVE is intended to be used for development tools only, and the PHIVE-installed tools are not expected to be shipped with the library package (hence the entries in the `.gitattributes), and they are also not expected to be deployed to production systems.

Dependabot currently does not update the PHIVE-installed tools yet.

@danon
Copy link
Member

danon commented May 27, 2021

I'm not sure how would I use it. Does it mean I have to have ./tools/phpcs.phar in my local clone?

@oliverklee
Copy link
Collaborator Author

@danon You'll need PHIVE on your local machine. You then can run phive install in your project to install the tools.

@danon
Copy link
Member

danon commented May 27, 2021

@oliverklee Wouldn't it be better if phive was in devDependencies?

@oliverklee
Copy link
Collaborator Author

@danon AFAIK, PHIVE is only available as a stand-alone PHAR, not as a Composer package (probably because it's a package manager itself).

@danon
Copy link
Member

danon commented May 27, 2021

I'm not sure about it.

If it was a devDependency, nobody would have to worry about it again.

With phive being something separate, when a build/sniffer fails; one to work on it, would have to go in, perhaps learn about phive for the first time.

It seems like too much complication to get a sniffer. Isn't there some other way to run the sniffer, something that could be a composer dependency?

@oliverklee
Copy link
Collaborator Author

@danon Thanks for the feedback! I've now changed this from a PHIVE-installed PHAR to a Composer dependency.

@danon
Copy link
Member

danon commented May 28, 2021

@oliverklee Would it be possible to only run the sniffer for the declare(strict_types) and for nothing else?

Remember, that we try to make this sniffer, so that we can introduce declare(strict_types) convention, (as of #16).

@oliverklee
Copy link
Collaborator Author

@danon Currently, the sniffer indeed only sniffs for the strict types, and for nothing else. :-)

So after this PR and #16 are merged, I can add the sniffer to the CI build.

(I'll add sniffing for PSR-12 later when all PSR-12 issues are resolved.)

@oliverklee
Copy link
Collaborator Author

@danon Is this ready to review, or do you still need anything from me for this PR?

@danon
Copy link
Member

danon commented May 28, 2021

@oliverklee I will review this today afternoon. Thank you for your help. If I don't find anything, I'll rebase the PR, if I do i'll write in this PR :)

@oliverklee
Copy link
Collaborator Author

@danon Do you still need anything from me so you can merge this PR? (I'd like to reduce the number of open PRs as I rebase all of them after one has been merged, and some PRs also depend on other PRs.)

@danon
Copy link
Member

danon commented Jun 7, 2021

@oliverklee I only have one question.

Why do we need two tools? Php_CodeSniffer and PhpStan?

Couldn't all our needs be solved by just one of them?

@oliverklee
Copy link
Collaborator Author

Why do we need two tools? Php_CodeSniffer and PhpStan?

Couldn't all our needs be solved by just one of them?

No, they focus on different things and have only a very small overlap concerning the issues each tool can find.

For example, PSR-12 style checking is a CodeSniffer-only thing, while data flow analysis with type compatibility check is a PHPStan-only thing. (And checking for the strict types declaration at the top of each PHP file also is a CodeSniffer-only thing.)

There now are the following new Composer scripts:

- `ci`: run all CI-related Composer scripts
- `ci:static` run the static code analysis
- `phpcs` run PHP_CodeSniffer
- `fix` fix the autofixable PHP code warnings
- `phpcbf` fix the autofixable PHP_CodeSniffer warnings

For starters, we have a sniff that checks that all files are in
PHP strict mode. (The CI builds do not run this check as the files
are not strict yet at this point.)

The PHP_CodeSniffer autofixer cannot make files strict, though
(as this is a non-autofixable code sniff). It will be able to
autofix other things we'll add later, though.
@danon
Copy link
Member

danon commented Jun 7, 2021

@oliverklee ok, so can we do the PhpSniffer for strict types first? And we leave PhpStan for the second step? :)

@oliverklee
Copy link
Collaborator Author

oliverklee commented Jun 7, 2021

Yes, we can do it in that order (This first step is what this PR is about).

@danon
Copy link
Member

danon commented Jun 7, 2021

I've pushed the commit into master.

@danon danon closed this Jun 7, 2021
@oliverklee oliverklee deleted the feature/code-sniffer branch June 7, 2021 20:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants