lina-usc / pylossless

🧠 EEG Processing pipeline that annotates continuous data
https://pylossless.readthedocs.io/en/latest/
MIT License
18 stars 8 forks source link

Project Linting Pass 1 #63

Closed Andesha closed 1 year ago

Andesha commented 1 year ago

As discussed in #48 - here we are!

Biggest effort here was restructuring the imports to be what pylint/pep8 wanted.

After that, all of the (mostly) major stuff in the pylossless folder should pass only with warnings now.

I've added several TODOs for docstrings but those aren't honestly a big deal and should never cause a CI to fail.

The thing that hasn't been addressed yet is that the pipeline.py file is too long by about ~200 lines according to the linter. This means that we're gonna have to cut some stuff out. I would suggest some of the Config structures as mentioned earler. That should likely become its own issue and an entirely different discussion.

:smile:

scott-huberty commented 1 year ago

Awesome!

I'll do my best on getting #57 in merged and then this.

The thing that hasn't been addressed yet is that the pipeline.py file is too long by about ~200

Totally. @christian-oreilly and I have discussed that this module needs to be split. It's nice to have someone else bring this up bc we've been neglectful during dev..

I also think, for a starter, our functions can be moved to a utils module, (and I think we should make the functions private).

But I am okay with handling the organization in a follow up PR if it makes more sense.

scott-huberty commented 1 year ago

Tackled the remaining flakes (flake8), docstyle (pydocstyle), and typos ( codespell) in pipeline.py

It is no nice now that pipeline.py does not light up like a christmas tree in the code editor.. Thank you @Andesha !

scott-huberty commented 1 year ago

I just made a few minor changes to the docstring because we use Numpy doc formatting (Since we use a sphinx tool to build the API doc automatically from the docstring, and it requires numpy or google formatting).

This LGTM. @Andesha shall we merge this and carry on the re-organization work in a follow-up PR?