Review code of NiiVue Jupyter notebook integration in IPyNiiVue about ipyniivue HOT 4 CLOSED

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

from ipyniivue.

Just seen there is some linting done in build.yml. I'll look it up and integrate any addition step that could be useful.

from ipyniivue.

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? :)

from ipyniivue.

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.

from ipyniivue.