Pre-commit hooks and black code formatting

[ngoiz]

Hi Team,

Another one from me...maybe this one is controversial

I've added some standard pre-commit hooks to SHARPy in order to ensure code consistency and formatting, such that code reviews can be spent reviewing code and not formatting!

What are pre-commit checks? These are checks that run every time you commit your code and ensure that the formatting adheres to standard practice.

Black is a formatter that will modify the .py files to ensure the format is consistent (i.e. line length, end of file etc.). Don't worry, it won't break the code as it checks that the python byte-content is the same before and after the operation.

As you can see, every single file in the repository has been modified to ensure consistent formatting. To avoid issues with git blame there is a new .git-blame-revs-ignore file that contains these massive commits so that the history of the file is unobstructed by these changes.

What is controversial? Black uses always double quotes for strings " rather than ' so every string in the code has been modified to this practice. This can be of course reverted.

Black is only one stage of the process that formats the code. The next step would be using flake8 that actually checks for various things before allowing a commit. These checks can be very strict (preventing commits with unused variables, unused import statements, etc.) which the tool cannot change automatically since it is modifying code. Therefore, when I tried using flake8 a lot of modifications are needed in the code files for the checks to pass which could be a project on its own making the code compliant. It could be worth it in the long run though, we could start with many exclusions (errors you don't check for) and adding them step by step.

In any case let me know what you think about including these in SHARPy, additionally we'd just need to add a line to the CI pipeline to run these alongside the tests.

I have manually added pre-commit to the new environment file only. Not sure if you are looking to discard the others.

# See https://pre-commit.com for more information
# See https://pre-commit.com/hooks.html for more hooks
repos:
-   repo: https://github.com/pre-commit/pre-commit-hooks
    rev: v3.2.0
    hooks:
    -   id: trailing-whitespace
    -   id: end-of-file-fixer
    -   id: check-yaml
    -   id: check-added-large-files
-   repo: https://github.com/psf/black
    rev: 22.10.0
    hooks:
    -   id: black

[codecov-commenter]

Codecov Report

Merging #224 (4581e85) into develop (3e5efc7) will increase coverage by 0.20%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop     #224      +/-   ##
===========================================
+ Coverage    67.14%   67.34%   +0.20%     
===========================================
  Files          155      155              
  Lines        25933    25878      -55     
===========================================
+ Hits         17413    17428      +15     
+ Misses        8520     8450      -70     

Impacted Files	Coverage Δ
sharpy/solvers/rigiddynamiccoupledstep.py	56.25% <0.00%> (-4.13%)	⬇️
sharpy/solvers/initialaeroelasticloader.py	46.77% <0.00%> (-2.46%)	⬇️
sharpy/solvers/nonlineardynamic.py	54.16% <0.00%> (-1.84%)	⬇️
sharpy/utils/exceptions.py	64.40% <0.00%> (-1.64%)	⬇️
sharpy/solvers/nonlineardynamicprescribedstep.py	81.48% <0.00%> (-1.57%)	⬇️
sharpy/solvers/statictrim.py	88.58% <0.00%> (-1.47%)	⬇️
sharpy/solvers/prescribeduvlm.py	59.25% <0.00%> (-1.11%)	⬇️
sharpy/linear/src/gridmapping.py	86.66% <0.00%> (-0.98%)	⬇️
sharpy/solvers/nostructural.py	56.81% <0.00%> (-0.96%)	⬇️
sharpy/solvers/trim.py	34.08% <0.00%> (-0.88%)	⬇️
... and 117 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[sduess]

Hi Norberto,
Thanks a lot for still contributing and this proposal. I find your idea to include black very good. On the weekend, my brother wanted to introduce me to black randomly as well. I showed him your YAML file and he added some stuff, he thinks might be useful as well (including the controversial flake8). Also, he applied the suggested syntax from YAML of using two spaces for indentation instead of four.
We will surely have a more detailed discussion in the next SHARPy team meeting!

[ngoiz]

Hi Stef,

Yes, I think it will be quite nice and preserve well the code in the long run with many contributors. It is going to be a bit of a hassle for everything to be compliant with flake8 as it doesn't let you commit with errors, but if it is done in chunks I think it is totally worth it.

Let me know how you decide to proceed in case I can offer a hand in reformatting parts of it!