PlanBCode / hypha

1 stars 8 forks source link

Automated code and style checks #338

Open matthijskooijman opened 4 years ago

matthijskooijman commented 4 years ago

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:

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 commented 4 years ago

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 commented 4 years ago

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