-
Notifications
You must be signed in to change notification settings - Fork 8
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Review code of NiiVue Jupyter notebook integration in IPyNiiVue #36
Comments
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 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) 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 |
Please review code to ensure Pythonic paradigms.
The text was updated successfully, but these errors were encountered: