Skip to content
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

Enforce mypy typechecking in CI #332

Closed
chfw opened this issue Aug 2, 2019 · 4 comments
Closed

Enforce mypy typechecking in CI #332

chfw opened this issue Aug 2, 2019 · 4 comments
Assignees
Labels

Comments

@chfw
Copy link
Contributor

chfw commented Aug 2, 2019

Hi all,

This project's PR template reads very interesting:

I accept that @willmcgugan may be pedantic in the code review.

I wondered if adding flake8 check to such a cool project would support the word: 'pedantic', strongly. Is there a good reason not to have flake8?

Here is what flake8 tells about the files:

...
tests/test_path.py:183:22: F405 'iswildcard' may be undefined, or defined from star imports: fs.path
tests/test_path.py:184:22: F405 'iswildcard' may be undefined, or defined from star imports: fs.path
tests/test_path.py:185:22: F405 'iswildcard' may be undefined, or defined from star imports: fs.path
tests/test_path.py:186:26: F405 'iswildcard' may be undefined, or defined from star imports: fs.path
tests/test_path.py:187:26: F405 'iswildcard' may be undefined, or defined from star imports: fs.path
tests/test_path.py:188:26: F405 'iswildcard' may be undefined, or defined from star imports: fs.path
tests/test_path.py:204:30: F405 'relativefrom' may be undefined, or defined from star imports: fs.path
...
@willmcgugan
Copy link
Member

I've found flake8 to have too may false positives as it only has a superficial understanding of the code. We do have typechecking in the project with mypy, which is far better at detecting issues in my experience. For instance mypy would know the difference between a star import and undefined symbol.

We don't have the mypy check in the CI though, and we really should. A few mypy errors have crept in since I last checked, so that needs to be addressed.

@chfw I'm going to accept this issue, but change the title...

@willmcgugan willmcgugan changed the title add style checker flake8? Enforce mypy typechecking in CI Aug 3, 2019
@willmcgugan willmcgugan self-assigned this Aug 3, 2019
@willmcgugan
Copy link
Member

Mypy issues fixed in master.

@chfw
Copy link
Contributor Author

chfw commented Aug 4, 2019

Thanks for accepting my issue. Yes, mypy definitely will help enforce the type check and help reducing runtime erros. 👍

With your attention, please allow me to explain a bit on the positive side of flake8. I am pleased already and I am not going to push like a nut :). That's only me so I would not like to push everyone.

My emacs or pycharm will flag up lines to indicate potential problems. Normally, you would like to have no red lines at a glance. Take tools.py as an example, the IDE tells it has some problems: imported but not used.

Screenshot 2019-08-04 at 20 25 00

Yet again, let me stress that my thanks for accepting my feedback. I am pleased that you took further action. It is appreciated!

@willmcgugan
Copy link
Member

I'm all for linting in the development environment. I use pylint in VSCode, which for some reason isn't picking up unused imports.

@dargueta dargueta mentioned this issue Aug 28, 2019
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants