add code quality tools #3
Merged
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overview
This is a fairly major PR to integrate several tools to aide in code quality and consistency. After this is agreed to and merged, with modifications as necessary, I'll follow up with another PR to add CI (continuous integration) to perform some checks when new PRs are opened. Below I lay out my reasoning for incorporating each tool and explain which options I've configured for each.
A quick note on configuration files --- all of the tools below have their own custom configuration files, but many tools in the python community have also moved towards allowing configuration via the
pyproject.tomlfile and/or thesetup.cfgfile. Some people prefer each tool to have its own custom/separate configuration file, but I personally hate having a ton of extra configuration files and thus I prefer using the two aforementioned files for configuration in any tool that supports them, with a preference towards thepyproject.tomlfile since it is more often supported.Also, it is worth noting that each of these tools have global options set via the various configuration files, but they all support locally disabling them (in an entire file or in certain lines of the file) via comments in the source files as well. In the interest of not making this PR exceedingly long, I'll let you read their documentation to find out how.
I'll now briefly describe each tool and my thoughts, but be sure to read the
Setupsection for a quick rundown on how you'd use this in practice. It is also worth mentioning that these tools are supported in most editors, such as VSCodium, vscode, PyCharm, etc. So you can format on save or by shortcut, get linting issues reported in real time, etc.Setup
I've included some new setup instructions in the
CONTRIBUTING.rstfile regarding how to get up-and-going with poetry (via pipx) for anyone unfamiliar with it, so please do see it. I will include here the basics to get started, assuming that you don't have pipx nor poetry already installed.Custom
runnerscriptThere is a file in the top directory simply named
runner, which is a bash shell script. It can be used on windows as well viagit-bashif you've installed Git for Windows. Make should also be installed separately when used on Windows so that the docs build. This script simply provides a single interface to run the tools below to fully check the code passes each tool as well as the tests. The idea (hope) is that if you pass all checks from this script, then you should pass the CI checks when you make a PR (CI checks to come later in a separate PR). I won't go through all of the details here as you can just run./runner --helpto see the options, or read the file itself which is rather simple and straight forward.Poetry
Someone mentioned in a recent issue or PR the importance of being able to exactly re-produce an older copy of section-properties, especially since it could be used in real engineering applications. Poetry is a great tool to explicitly lock the entire dependency tree, including all sub-dependencies, via the poetry.lock file. Thus with this file, anyone could grab an old release and exactly reproduce the entire environment (if and only if installed using the lock file --- installing with
pipfrom PyPI does not know about and does not use the lock file!). During developement, we can use this lock file to ensure that we all have identical virutal environments setup, which may aide in finding issues that only some of us experience. It is important to note though that when installed via PyPI, the lock file is not used and the exact versions of libraries the user has will thus be different/uncertain.Poetry requires use of the
pyproject.tomlfile. The recommended way to use Poetry is to install it outside of your virtual environment (I like to use pipx for this) and then use Poetry for the entire project including creation and setup of the virtual environment.blackblack is the infamous "opinionated" python code formatter. I can assure you that you'll fight it at first because it will format some things differently than your preference. Being opinionated, it intentially provides very few configuration options, so you basically have to just accept what it does, or not use it. The intention is to just use black and not spend time arguing or deciding what the best formatting standard to follow is. After you get used to it, you'll likely love it and use it all the time and even grow to appreciate some of the "weird" formatting you didn't like at the start.
black is only configurable via the
pyproject.tomlfile. As I mentioned, there are very few options that you can change, but two of them that I have adjusted here are:isortisort is used to sort your imports. Best practice is generally to put standard library imports, then third party libraries, and last your own first party imports. Unlike
black, isort has quite a few configuration options. Some of them need to be set to specific values to makeisortcompatible withblack, but others can still be adjusted. For example, it can sort each "section" by alphabetical order (default) or by line length (optionlength_sort).Starting with version 5.0.0, isort introduced "profiles" to automatically set certain options. One of them is a
blackprofile so thatisortandblackwork nicely together.I've setup isort configuration in the
pyproject.tomlfile and it has just a few options set:blackflake8+ friendsflake8wraps a couple of tools and is a very quick and useful tool to find common linting issues and recommended code improvements. It also supports plugins or extensions to add additional checks. I've included here the plugingflake8-docstringwhich adds an extension to use pydocstyle to enforce improved docstring consistency. I've also addedflake8-bugbearwhich includes some additional checks and bug finding features. Finally, I've addedflake8-rst-docstringswhich can be used to check that the docstrings contain valid reStructuredText.I've setup
flake8configuration in thesetup.cfgfile. flake8 is fairly configurable, but I've only made a few changes to the defaults:black) plus some additional ignores that we should eventually remove (but I don't want to make too many code changes to fix all flags in 1 PR)pylintpylintis a linter + static code analysis tool that is quite a bit more complex thanflake8, usually more noisy, and significantly slower. It supports a huge amount of customization though to allow only using the parts of it that you want. The static analysis part catches some poor idioms and errors that flake8 cannot detect, so I do like to use it in addition toflake8.I've setup
pylintconfiguration in thepyproject.tomlfile.pylintoffers a huge amount of configuration and you could probably spend a solid weekend looking through it all. I've only changed a few here:I still get false positives with
pylintor have certain functions or branches of code that I want to locally disable specific checks for. Although all of the above tools support local disables in code, I rarely use them except forpylint. These are usually via a comment# pylint: disable=option-name-here. I find withpylintthat the big decision for it comes down to which checks you decide to put in the global disable list and which ones you want to only disable occasionally within the code via comments.rstcheckrstcheckis a tool to perform checks on rst files to ensure they are valid. Note that it has not been updated in a few years, so this might be a dead project. Nevertheless, it works well enough for now.Configuration of this tool is in the
setup.cfgfile.pre-commitpre-commitis a package to install git hooks to perform checks on code before it is committed. Regardless of whether this is used, whatever requirements are agreed upon should be enforced via CI, but I like using pre-commit git hooks so that I can fix issues locally before even bothering to open the PR and having the CI fail. Even after installation, you can ignore the hooks if needed via the no verify option to git,git commit --no-verify ...Configuration for this is in the
.pre-commit-config.yamlfile. The normal, recommended way to use these hooks are to point to the git repository for each tool you use and let the python pre-commit tool make virtual enviornments and manage those tools. I don't like this idea as then you have to maintain the version for each dev dependency in two locations (this config file and the pyproject.toml file). Plus, if I'm using the tool as a dev dependency anyway, then it is pointless to make another environment when my local project environment already has the correct tool and correct version installed. Thus, I've set all of these hooks to usesystemandpoetryto run the tool from the local virtual environment instead. So if you look into the documentation on this tool and see configurations a bit different than what I have, that is why.pytestI believe we've all agreed to incorporate
pytestfor testing, which is very common in the python community.I've puts its configuration into the
pyproject.tomlfile consistent with the other tools above.Other Tools (Not Used)
There are many other tools available that could potentially be used in addition, or in place of, the tools I've incorporated and mentioned above. My selection of the above tools is certainly based on my own bias, as they are what I regularly use in other projects. However, I also feel that they are the most common ones used in other python projects I've encountered or been involved with. Nevertheless, for posterity, I'll briefly mention a few others. Note that my experience with most of these is minimal:
pydocstyle- This actually is used via theflake8-docstringplugin and provides docstring checkingmypy- performs static checking and I think that we should use and incorporate this when (or if) we update section-properties to use type annotations as without them this tool is not useful - I've actually included an initial configuration for it in thesetup.cfgfile, but it isn't currently useddodgy- tries to help prevent you accidentally committing code with personal information like passwords, secrets, etcprospector- brings together a few tools under one project, but all are already covered by using pylint and flake8pylama- annother conglomerate tool to bring together various tools into 1 interface, but is also mostly covered by those already usedpychecker- finds bugs, I think via a similar method to pylint (I've never used this one)bandit- an interesting tool that looks for security vulnerabilities within your codeRadon- tool to compute some various metrics on your code, such as an estimate of how maintainable it is, how complex it is, etc - this one seems pretty interesting, but I've never used iteradicate- removes commented out code, as it is generally considered poor practice; with git you don't need that old commented out code!flake8plugins -flake8can easily be extended and there are many, many plugins (extensions) available forflake8, which can bring multiple tools under it. We've already used 2, but there are countless others including plugins for some other tools mentioned previously (i.e.eradicate,bandit, andisort). Whether you use a tool separate or underflake8is really a matter of personal preference. I prefer to keep isort separate (it is a formatter, not a linter), so I've put it separate. I could also have kept pydocstyle separate, but I do like its integration withflake8.