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

Automated code and style checks #338

Open
matthijskooijman opened this issue Aug 19, 2020 · 2 comments
Open

Automated code and style checks #338

matthijskooijman opened this issue Aug 19, 2020 · 2 comments

Comments

@matthijskooijman
Copy link
Contributor

This is split off #132, so that issue can remain about actual (unit) tests that actually run Hypha code, and this issue can be about automatic code (style) checks (i.e. static analysis) using third-party tools.

Previously, I wrote:

One type of testing is static analysis or linting, where no code is actually run, but a script looks at the code and checks for common mistakes. There's roughly two things that these scripts can check:

  1. Code formatting: Whether the code complies with certain style guidelines
  2. Bad practices: Certain constructs or bits of code that are considered bad practice
  3. Code checks: Whether the code contains mistakes (e.g. references to undefined functions, variables, etc.)
  4. Type checks: Whether the types of expressions are correct (e.g. not passing a string where an int is expected). This is something that's enabled by recent improvements in PHP where variables and parameters can be annotated by their type, though often types can be derived as well.

I've looked at a few of these analyzers:

  • Psalm is a fairly new one, that seems to focus on code and type checks. It is quite configurable with different strictness levels, not sure if it's easy to add your own checks. It supports a "baseline file", to exclude any existing problems making it easier to gradually improve by (somewhat) only scanning newly added code.
  • PHPCS (code sniffer) is a scanner for (exclusively, it seems) code formatting. It does seem easy to add specify a customized style, reusing existing "sniffs" and adding your own. It also supports automatically fixing most issues (though that might end up producing a mess, of course).
  • PHPMD (mess detector) seems to scan mostly for bad practices.
    Note that this is all different from actual testing, where you run custom-written (Hypha-specific) testing code that actually runs Hypha code and evaluates whether the output is as expected.
  • PHPSTAN (static analysis) mostly does code checks (and maybe some limited type checks). It is quite configurable with different strictness levels, custom checks also seem to be possible. Also supports a "baseline file".

It seems that Psalm might have the most extensive code and type checking. PHPSTAN seems a bit less complete, but has nice output and can be easily integrated into my editor (vim, using syntastic), so I'll probably be enabling PHPSTAN and maybe see how much issues it misses that psalm ort PHPMD find. We could maybe add PHPCS for code formatting checks, once we decide what we want in this area.

@matthijskooijman
Copy link
Contributor Author

I've tried PHPSTAN for a bit, but it is not ideal. I ran into some problems running it from my editor (it is apparently not intended to run for one file, but should be ran on the entire project). It is also quite slow (12s to run on the entire project, 4s for one file).

Given that Psalm also seemed to have more extensive checks, turns out to be a lot faster (<1s for a single file), supports checking single files and integrating it into my editor turned out to be rather easy, I've now switched to psalm, which has already caught quite a few typos and other small oversights in the code I was writing.

Good next steps could be to fix the remaining errors, or establish a baseline (but there's only 64 errors at level 5, most of which are probably problems with psalm not seeing all code properly rather than actual bugs, so fixing is probably better) and then enable automatic checking of all code for all pullrequests and pushes.

From there, we could try increasing the strictness (https://psalm.dev/docs/running_psalm/error_levels/) and fix more of those issues and/or establish a baseline again, so new code can be subjected to more checks.

Eventually, I'd like more strict checks for missing type annotations, but that probably requires a newer PHP version so we can actually use native annotions rather than resorting to comments for a lot of annotations (See #302).

@matthijskooijman
Copy link
Contributor Author

If we apply a style checker, we might want to include checks for consistent naming (e.g. #324).

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

No branches or pull requests

1 participant