ENH: Automatically fix a number of formatting issues using pre-commit

[Patol75]

For this PR, I have used the pre-commit framework to automatically check and correct formatting issues within the main branch of Fluidity. Hooks used are listed in .pre-commit-config.yaml and detailed here. I have only encountered two issues. The first one was with Makefile.dependencies files, which show a conflicting end-of-file behaviour when they are generated through make makefiles. The second one is with regard to a binary VTU file for which I have not investigated further. None of these two issues seems significant to me.

pre-commit also provides a CI solution, but unfortunately, it is not available for free for GitHub organisations. In any case, I believe the automatic changes included here will be helpful with future development of Fluidity as formatting should not be an issue anymore.

[stephankramer]

This is great. Tried it out for myself and seems to work nicely and non-intrusively.

[angus-g]

Looks like a good idea. I think the CI Action should still be available (it's for "open source repositories"), it would just need a FluidityProject admin to install it, I assume.

[Patol75]

Thank you @angus-g and @stephankramer.

@stephankramer Any chance to try the suggestion of @angus-g? You just need to log in using Fluidity's GitHub account here and see if it lets you install pre-commit for the repository for free. I have done it with my repositories, and it was pretty straightforward.

[angus-g]

Looks like the change to vtutools.py is breaking CI:

diff --git a/python/fluidity/diagnostics/vtutools.py b/python/fluidity/diagnostics/vtutools.py
index ad7c4ba0a..f89798a05 100644
--- a/python/fluidity/diagnostics/vtutools.py
+++ b/python/fluidity/diagnostics/vtutools.py
@@ -28,8 +26,9 @@ import fluidity.diagnostics.elements as elements
 import fluidity.diagnostics.optimise as optimise
 import fluidity.diagnostics.simplices as simplices
 import fluidity.diagnostics.utils as utils
+import numpy as np
 import vtk
-from vtktools import *
+from vtktools import VtuMatchLocations, vtu

 def VtkSupport():

Lots of tests import vtutools as vtktools, which is a bit confusing!

[Patol75]

That is a good break, it allows us to see which tests import vtutools as vtktools, making use of the now-deleted from vtktools import * within vtutools. I am pretty sure I have already fixed these issues, but very likely in another branch. I will wait until CI finishes, commit the additional changes to the tests, and we should be good to go.

[gnikit]

I had a go at it and it looks very good, great job @Patol75 a few things to maybe consider:

1. Change reorder_python_imports with isort, we will also have to set the profile to profile = "black"

-   repo: https://github.com/pycqa/isort
    rev: 5.10.1
    hooks:
    -   id: isort
        name: isort (python)

2. Add flake8 for linting

   -   repo: https://github.com/PyCQA/flake8
   rev: 4.0.1
   hooks:
   - id: flake8

   We would also have to setup these 2 rules for flake8

max-line-length = 88
extend-ignore = E203, E722

Also, I did a sanity check that

find -name '*.flml' -exec spud-update-options -s schemas/fluidity_options.rng {} \;

does not update the .flml files in a way that would be incompatible with pre-commit. It all checks out.

[Patol75]

CI was all green before the last commit, so should be all good with vtutools.

Thanks, @gnikit for the comments. I had indeed first tried isort, but I was unaware of the compatibility arguments with black, hence why I had tried another hook. That should be sorted now. For flake8, well, I had already tried it too, but given it only highlights problems and does not fix them, I had left it out. Oh well, now it is in, and it does not seem to create much trouble. Also, thanks for the sanity check!

Given the latest commit here, I see that pre-commit is now part of the CI. Thanks, @stephankramer. However, this brings back the issue with tests/data/ref_vtk_hexahedra.vtu: expect a failure from unit tests. Should we attempt to fix this file, or simply exclude it from the mixed-line-ending hook? The latter would look like that:

    -   id: mixed-line-ending
        exclude: ref_vtk_hexahedra.vtu

[gnikit]

I think I added that file a while back when unittesting the node ordering of the VTK routines. Do we know why the test fails? I am looking at a diff and the two files and they look the same to me.

[stephankramer]

I suspect it's because there's "raw" binary data in the file which happens to have a byte in that corresponds to an end-of-line character that's different from the ones used in the ascii part of that file - thus tripping the mixed-line-endings check.

I suppose you could fix by changing the encoding to base64, or exclude it - I don't really care.

In general I would say that pre-commit stuff should mostly only touch human edited files. If it trips on generated files that are generated by our code, we can fix the code to only produce compliant files. If it's produced through third-party tools I'd say we should aggresively exclude those from the tests. So say for instance gmsh produces non-complaint .msh files (it appears this is not the case) we just exclude *.msh and if we find more vtus that are causing issues we just exclude all of them from the pre-commit tests.

In particular I don't want to be in the situation were a file produced by some tool, say diamond writing a .flml, gets "fixed" by a pre-commit hook (but as @gnikit has checked it seems this won't be the case) and then you open it up and save it in that same tool again and you obtain local changes (which would be fixed again by pre-commit). We either fix the tool or exclude all such files.

[stephankramer]

@gnikit : out of curiosity, any particular reason you prefer isort over reorder_python_imports?

[gnikit]

@gnikit : out of curiosity, any particular reason you prefer isort over reorder_python_imports?

Substantially wider adoption from projects so less likely to be abandoned (isort 460M vs reorder_python_imports 831k). reorder_python_imports is owned by an individual whereas isort is owned py PyCQA

[Patol75]

CI should now return all green on the building and testing sides, but pre-commit is unhappy because the compile symlink in libspatialindex is broken. I updated it in the original commit because it was broken in both Focal and Jammy, but now it looks like it is broken in the CI. Am I missing something, or should I just exclude that file from the broken-symlink hook?

[stephankramer]

Yeah, I'm not sure I ever intended to commit that symlink. Looks like an odd thing to do ¯\_(ツ)_/¯ There's still the flake8 stuff of course, which I think is a good addition but we may need some more exceptions (after fixing the obvious)

[Patol75]

Thanks, @stephankramer. I have merged the warnings branch here and started to address the flake8 complaints. There are quite a lot of them, so that is going to take some time. Any help appreciated.

EDIT: Can anyone confirm they are also seeing this run as In Progress? If yes, could someone with proper rights stop it? I am not too sure what is going on there...

EDIT: I have deleted the entire log for the run I mentioned above, just to be sure nothing weird was happening with the Actions resources. Re-triggering the case that failed here, it passed, but then the run is sort of hanging as well... Anyway, it looks like merging the two branches was a success.

[Patol75]

PR should now be good to go.

[Patol75]

Thanks for the changes @stephankramer.