GIScience / orstools-qgis-plugin

Plugin for QGIS with a set of tools to use openrouteservice API´s, based on openstreetmap
https://plugins.qgis.org/plugins/ORStools/
MIT License
91 stars 31 forks source link

Introduce automated code style formatting and linting #141

Closed koebi closed 7 months ago

koebi commented 3 years ago

The current state

Currently, this project does not have any default code style, neither is there any static code analysis in place. I am strongly convinced that introducing both would result in better, easier maintainable and easier-to-contribute-to code.

What I think we should do

I'd like to use black as a code formatter and flake8 and pylint for static code analysis.

What that would fix

$ flake8 | wc -l
461

$ pylint ORStools
…
Your code has been rated at 3.19/10

$ black ORStools
…
29 files reformatted.

Notes

Git-blame supports ignoring specific revisions - one big reformatting-commit could be ignored by any git-blame command run on the repo.

koebi commented 3 years ago

In any case, #133 should get done beforehand.

TheGreatRefrigerator commented 3 years ago

Need to check correct settings for all the exceptions like wrapper classes using camel case for method names and similar. Not that straight forward in this repo due to c++ dependencies.

koebi commented 3 years ago

This should also fix relative imports all around.

Additionally, note that .ui and …UI.py-files are automatically generated and should not be touched.

koebi commented 3 years ago

We have a few type annotations in some places. We should probably expand on this and use mypy for type checking at some point.

koebi commented 3 years ago

Regarding line length: This is a count of how many lines exceed the flake8 default line length of 79 characters.

$ flake8 --exclude __pycache__,*UI.py,*.ui,resources_rc.py | grep -o '[0-9]\+ >'|awk '{print $1}' | sort -h | uniq -c
      6 80
     12 81
     16 82
      8 83
     11 84
      7 85
      8 86
      6 87
      8 88
     10 89
      8 90
      9 91
      5 92
     10 93
      4 94
      9 95
      4 96
      3 97
      9 98
      2 99
      6 100
      3 101
      3 102
      6 103
      2 104
      3 105
      5 106
      2 107
      6 108
      3 109
      2 110
      5 111
      3 112
      5 113
      4 114
      4 115
      5 116
      3 117
      3 118
      1 126

In total, these are 229 out of roughly 8000 lines. Of these, 82 have line length <= 88 (black default line length).

On line length, there's a great piece of talk by Raymond Hettinger, python core dev. He recommends 90-ish as line length - meaning to not worry too much if it inches over the edge by a few characters, but rather to take it as a hint that maybe something is going wrong.

I'd therefore opt to look into the ~70 Lines w/ a line length over 100 and maybe into the other 50 w/ line length between 88 and 100 :)

TheGreatRefrigerator commented 3 years ago

We have a few type annotations in some places. We should probably expand on this and use mypy for type checking at some point.

Did you know this is built in in PyCharm? (Hint)

TheGreatRefrigerator commented 3 years ago

Main point for character limit is to be able to have files open next to each other and still be able to read the code properly. With resolution of our monitors in addition to multi monitor setup, i would maybe opt a bit higher than the original 70.

I would rather opt for a hard limit than a 90-ish rule, as i can set the character limit within the IDE and it will automatically break at the limit and notify if lines are over the limit.

(still got it set to 120 which is the default taken over from intellij)

TheGreatRefrigerator commented 3 years ago

how do you feel about a hard limit of 100 chars?

koebi commented 7 months ago

We could set this up as proposed in the black documentation here, using github actions.

The same Lint workflow could also include pylint, flake8 and possibly isort. Compare the Django workflow for this.

Local installations would have to run them manually (or wait for any PRs to go through linting) - or we use any form of git hooks, possibly via pre-commit.

koebi commented 7 months ago

After further reviewing, we'd probably prefer ruff. ToDo: