Closed cdrake closed 5 months ago
I'll have a look at this. Let's add to this issue the integration into the CI of some linting validation at the same time. We can probably re-use directly the yaml file from the PyLossless project for that:
name: PEP8 Compliance, Spellcheck, & Docstring
on: pull_request
jobs:
linting:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v3
- name: Set up Python 3.x
uses: actions/setup-python@v4
with:
python-version: "3.x"
- name: Install dependencies
run: pip install -r requirements_testing.txt
- name: Run flake8
run: flake8 pylossless docs --exclude pylossless/__init__.py
- name: Run Codespell
run: codespell pylossless docs --skip docs/source/generated
- name: Check Numpy Format Docstring
run: pydocstyle pylossless
Just seen there is some linting done in build.yml. I'll look it up and integrate any addition step that could be useful.
I'll note some issues here as I see them. I'll correct them as I see them and may end up merging these corrections through different PRs.
First note: mutable objects should never be given as default values. It can cause side effects that are very hard to detect.
For example, form nvmesh.py
class NVMesh:
#gl variable skipped because python
def __init__(self, input_dict={}, **kwargs):
kwargs.update(input_dict)
This would be better replaced with For example, form nvmesh.py
class NVMesh:
#gl variable skipped because python
def __init__(self, input_dict=None, **kwargs):
if input_dict is None:
input_dict = {}
kwargs.update(input_dict)
Why is it nasty?
What the hell, right? :)
Second note: This is not a Python thing specifically, but if-elif without else can be somewhat dangerous, depending on the context. An example of that is the add_volume
method in niivue.py
. I was calling it with a Path object, and it was failing silently, just doing nothing. In this function, there is a block of code with an if-elif statement that processes the different types... but no else clause. So, this function was failing silently leaving the user puzzled about what was happening. The correct thing to do here is to add an else
block with a raise TypeError("The argument volume of blablabla accepts only arguments of types blablabla")
... I'll fix this one in my PR for issue #23 if I ever manage to get this working.
Please review code to ensure Pythonic paradigms.