search

iobis / pyobis

OBIS Python client
https://iobis.github.io/pyobis
MIT License
15 stars 10 forks source link

Add flake8 Linting #35

Closed 7yl4r closed 2 years ago

7yl4r commented 2 years ago

This PR will add flake8 linting as a github action.

There are many linting issues which we should resolve before merging this in.

tylar@laptop:~/pyobis$ flake8 ./pyobis/ | wc -l
477
7yl4r commented 2 years ago

I don't know why I am getting no module Flake8. I have tried several things to try to ensure its installation. The environment handling in GitHub actions confuses.

Maybe it makes sense to dig into #16 before going farther into debugging that.

ayushanand18 commented 2 years ago

I tried working on it, and ran an experiment. Actually, we were getting module not found error, because maybe we forgot to activate the conda environment in which we installed all dev dependencies before running the Linter.

I did some changes and ran a test, and the Linter ran perfectly fine. Here's the branch, the file and the run

But it exited with status code 1 and threw an error. So tests look as if failed. This may be because it pointed out so many issues and to be honest, I am really not sure. Should we work on refactoring code based on the issues it pointed out?

ocefpaf commented 2 years ago

@7yl4r and @ayushanand18 I'm a big fan of pre-commits and pre-commit-ci. We can add all the linting we want in a YAML config file like this one. And then activate the pre-commit-ci to check them every PR. The beauty of it is that, if the user has pre-commit installed locally, the github commit hook will fix the code before pushing. If not, b/c not all users want to use it, the CI will fail and the user can amend the PR manually.

ayushanand18 commented 2 years ago

@7yl4r and @ayushanand18 I'm a big fan of pre-commits and pre-commit-ci. We can add all the linting we want in a YAML config file like this one. And then activate the pre-commit-ci to check them every PR. The beauty of it is that, if the user has pre-commit installed locally, the github commit hook will fix the code before pushing. If not, b/c not all users want to use it, the CI will fail and the user can amend the PR manually.

This is really great! It will help reviewers invest their time on what matters the most. I am not very much aware of pre-commits but I really like the idea behind them.

ayushanand18 commented 2 years ago

Some of the linting tests failed and I don't if they are okay like this one. 

pyobis/dataset/__init__.py:1:1: F401 '.dataset.get' imported but unused
pyobis/dataset/__init__.py:1:1: F401 '.dataset.search' imported but unused

Aren't these necessary for the module to function otherwise the functions might not be locatable? Please correct me if my understanding is wrong.

7yl4r commented 2 years ago

Some of the linting tests failed and I don't if they are okay like this one.

pyobis/dataset/__init__.py:1:1: F401 '.dataset.get' imported but unused
pyobis/dataset/__init__.py:1:1: F401 '.dataset.search' imported but unused

Aren't these necessary for the module to function otherwise the functions might not be locatable? Please correct me if my understanding is wrong.

You only need to add the __all__ attribute to fully comply with PEP8.

To better support introspection, modules should explicitly declare the names in their public API using the all attribute.

example:

from .my_class import MyClass

__all__ = ['MyClass',]
7yl4r commented 2 years ago

I am closing this, since pre-commit handles this more elegantly than my proposed solution.