This PR adds flake8, along with several flake8 plugins to further enhance the rules which the codebase should be following.
Specifically, it adds these flake8 extensions:
flake8-bugbear: A quick way to catch non-pythonic and generally bad patterns in code, which may work and pass the tests, however they aren't easily maintainable and often cause issues in the long-term. These are things like bare exceptions, disallowing us to set mutable structures as function parameter defaults (since it's unintuitive that if they're changed once, every new call will use that mutated structure, not a new one), Disallowing assert False since running the program with python -O removes these calls, and tons of other checks like these.
flake8-tidy-imports: prevents bad import patterns, such as from x import y as y or import x.y as y, optionally can also prevent relative imports, which I did enable here since our codebase mostly wasn't using those anyway and they can get messy. It also supports custom rules for disallowed imports, however I did not utilize that here.
flake8-import-order: This will probably be the most "controversial" one, as it can add a lot of work for contributors, since they now need to make sure their imports will be in alphabetical order and in proper group (stdlib group is first, then external imports, then internal project imports). But I think it's worth it since it does bring a lot of standardization and we can immediately know where to look for an import. A lot of editors also have a "sort imports" feature, which can do this for you, making it a bit less annoying. Specifically, the import order style I've choice is the pycharm style, but if desired, it can be changed to one of other supported styles, however in my experience, I found pycharm's import ordering style to be really easily maintainable due to this group separation, and the case sensitive alphabetical order in it makes it easy to quickly orientate in that group as well.
pep8-naming: This extension enforces proper conventional variable/function/class naming for python (constants should be in SCREAMING_SNAKE_CASE, variables and functions in snake_case and classes in PascalCase), there are also some other naming rules listed in their README.
flake8-annotations: This extension enforces the use of type annotations in functions. I've added some exceptions for things that we will not be annotating in this project, such as the requirement to annotate self or cls parameters (this is pointless and just annoying, pyright can determine these types automatically), annotating *args and **kwargs (this is sometimes useful, if for example we know our *args should only be strings, however I don't think these annotations should be required), and return annotations on special methods, such as __init__ (being None), or __new__, or __bool__, etc. as these will pretty much always stay the same and pyright will know them without us needing to specify them (though it obviously doesn't prohibit it, sometimes it may be useful if for example our __bool__ function wouldn't be returning a bool type, but rather only Literal[True] for example.)
Addressing project's docstring convetion
There were some discussions about project's docstring convetion, which lead us to propose flake8 as something that may be used to enforce some convention like this, however it wasn't yet decided what convention will we use, and so this PR will NOT address this issue and the specific flake8 plugins and rules to do this should be added in another one.
But I did want to mention 2 flake8 plugins which may be worth adding in future PRs:
flake8-docstrings: Very involved plugin which checks content of docstrings for several rules, however many of it's rules conflict with each other and so it requires a lot of ignores (because of that, setting it up is a bit annoying and can take up quite a bit of time, however it does allow everyone to fine-tune it exactly to their liking). It is a more general plugin which does have support for specific docstring conventions, but this only supports a few of those and we don't need to use it for that, however it is great in that it does allow us to do things like require imperative mood in docstrings, determine whether there should be a newline after a class docstring, after a fucntion docstring, determine whether to start the docstring on the line with """ or on the line below, and many other things like this.
flake8-rst-docstrings: If we're going to decide to go with the reST docstring
convention, this plugin can enforce it for us (though I don't have that much experience with it to describe it any further)
Tasks
[x] Add flake8
[x] Add some specific flake8 plugins
[x] Make the codebase compliant with these new formatting rules
[x] Run flake8 on tox lint along with pyright
[x] Run flake8 in the github workflow for each PR/push
[ ] Document this in CONTRIBUTING.md?
Some notes
Because of flake8-annotations requiring python >=3.6.2,<4.0.0, I've adding this restriction to our python version requirement as well. It's not a huge issue, however it will require everyone using our library to also make this change to their python version requirement.
This linter addition should be fully compatible with black, following the black docs, though I obviously haven't tested every possible situation, especially with the extra plugins, but at least for the current codebase, there were no issues with black.
While flake8-annotations plugin is being added by this PR, a lot of the main type annotation requirements are disabled, namely these are the two rules: ANN001 (general function parameter type annotation) and ANN201 (general function return type annotation), because the codebase is still missing a lot of these type-hints, and this PR is not a place to add them, I've marked these disabled flake8 rules as # TEMPORARY in .flake8 to make it clear that we intent to respect them at some point and so new code should still respect them, even though they're disabled for now.
All of the bare excepts found by flake8-bugbear were only addressed by making them except Exception, which doesn't really solve the underlying issue of not ignoring other exceptions and only excepting those which we absolutely need to, however since I don't know what these statements were supposed to be excepting, and this isn't a place to fix that, I've merely added an inline TODO comment mentioning this for each of these excepts.
Documenting flake8 usage in CONTRIBUTING.md was marked as a non-task, because CONTRIBUTING.md needs a much bigger overhaul, documenting how to use tox and other things and it didn't seem appropriate to only mention flake8 there, but mentioning more would be outside of this PR's scope, so another PR should address this later
General goal of this PR
This PR adds flake8, along with several flake8 plugins to further enhance the rules which the codebase should be following.
Specifically, it adds these flake8 extensions:
flake8-bugbear: A quick way to catch non-pythonic and generally bad patterns in code, which may work and pass the tests, however they aren't easily maintainable and often cause issues in the long-term. These are things like bare exceptions, disallowing us to set mutable structures as function parameter defaults (since it's unintuitive that if they're changed once, every new call will use that mutated structure, not a new one), Disallowingassert Falsesince running the program withpython -Oremoves these calls, and tons of other checks like these.flake8-tidy-imports: prevents bad import patterns, such asfrom x import y as yorimport x.y as y, optionally can also prevent relative imports, which I did enable here since our codebase mostly wasn't using those anyway and they can get messy. It also supports custom rules for disallowed imports, however I did not utilize that here.flake8-import-order: This will probably be the most "controversial" one, as it can add a lot of work for contributors, since they now need to make sure their imports will be in alphabetical order and in proper group (stdlib group is first, then external imports, then internal project imports). But I think it's worth it since it does bring a lot of standardization and we can immediately know where to look for an import. A lot of editors also have a "sort imports" feature, which can do this for you, making it a bit less annoying. Specifically, the import order style I've choice is thepycharmstyle, but if desired, it can be changed to one of other supported styles, however in my experience, I found pycharm's import ordering style to be really easily maintainable due to this group separation, and the case sensitive alphabetical order in it makes it easy to quickly orientate in that group as well.pep8-naming: This extension enforces proper conventional variable/function/class naming for python (constants should be inSCREAMING_SNAKE_CASE, variables and functions insnake_caseand classes inPascalCase), there are also some other naming rules listed in their README.flake8-annotations: This extension enforces the use of type annotations in functions. I've added some exceptions for things that we will not be annotating in this project, such as the requirement to annotateselforclsparameters (this is pointless and just annoying, pyright can determine these types automatically), annotating*argsand**kwargs(this is sometimes useful, if for example we know our *args should only be strings, however I don't think these annotations should be required), and return annotations on special methods, such as__init__(beingNone), or__new__, or__bool__, etc. as these will pretty much always stay the same and pyright will know them without us needing to specify them (though it obviously doesn't prohibit it, sometimes it may be useful if for example our__bool__function wouldn't be returning abooltype, but rather onlyLiteral[True]for example.)Addressing project's docstring convetion
There were some discussions about project's docstring convetion, which lead us to propose flake8 as something that may be used to enforce some convention like this, however it wasn't yet decided what convention will we use, and so this PR will NOT address this issue and the specific flake8 plugins and rules to do this should be added in another one.
But I did want to mention 2 flake8 plugins which may be worth adding in future PRs:
flake8-docstrings: Very involved plugin which checks content of docstrings for several rules, however many of it's rules conflict with each other and so it requires a lot of ignores (because of that, setting it up is a bit annoying and can take up quite a bit of time, however it does allow everyone to fine-tune it exactly to their liking). It is a more general plugin which does have support for specific docstring conventions, but this only supports a few of those and we don't need to use it for that, however it is great in that it does allow us to do things like require imperative mood in docstrings, determine whether there should be a newline after a class docstring, after a fucntion docstring, determine whether to start the docstring on the line with"""or on the line below, and many other things like this.flake8-rst-docstrings: If we're going to decide to go with thereSTdocstring convention, this plugin can enforce it for us (though I don't have that much experience with it to describe it any further)Tasks
Document this inCONTRIBUTING.md?Some notes
flake8-annotationsrequiring python>=3.6.2,<4.0.0, I've adding this restriction to our python version requirement as well. It's not a huge issue, however it will require everyone using our library to also make this change to their python version requirement.black, following the black docs, though I obviously haven't tested every possible situation, especially with the extra plugins, but at least for the current codebase, there were no issues with black.flake8-annotationsplugin is being added by this PR, a lot of the main type annotation requirements are disabled, namely these are the two rules:ANN001(general function parameter type annotation) andANN201(general function return type annotation), because the codebase is still missing a lot of these type-hints, and this PR is not a place to add them, I've marked these disabled flake8 rules as# TEMPORARYin.flake8to make it clear that we intent to respect them at some point and so new code should still respect them, even though they're disabled for now.flake8-bugbearwere only addressed by making themexcept Exception, which doesn't really solve the underlying issue of not ignoring other exceptions and only excepting those which we absolutely need to, however since I don't know what these statements were supposed to be excepting, and this isn't a place to fix that, I've merely added an inline TODO comment mentioning this for each of these excepts.CONTRIBUTING.mdwas marked as a non-task, becauseCONTRIBUTING.mdneeds a much bigger overhaul, documenting how to use tox and other things and it didn't seem appropriate to only mention flake8 there, but mentioning more would be outside of this PR's scope, so another PR should address this later