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

PEP-8 #92

Closed
wants to merge 5 commits into from
Closed

PEP-8 #92

wants to merge 5 commits into from

Conversation

c4xuxo
Copy link

@c4xuxo c4xuxo commented Aug 27, 2017

Changes made base PyCharm inspections, flake8 and also some feedback from pylint.

Goal was to fix as much "problems" as possible without (or with minimal) impact on code.
Some parts need refactoring and renaming of variables, methods, classes, although this was avoided (as much as possible) in this change.

Before

flake8 --count
774

PyCharm
image

pylint *

Your code has been rated at -2.97/10 (previous run: -2.97/10, +0.00)

After

flake8 --count
20

PyCharm
image

pylint *

Your code has been rated at -1.31/10 (previous run: -2.97/10, +1.65)

@tataganesh tataganesh changed the base branch from master to develop November 8, 2018 18:54
@pietermarsman
Copy link
Member

Thanks for the effort!

It has been a while since you have made these changes and they are not up to date anymore with the current branch. @c4xuxo, can you look into that and fix it?

Another thing, perhaps we want to add flake8 testing to the test suite. I've experience with pytest-flakes, but perhaps there is an alternative for nose. Or just a seperate step in the testing process. @ganeshtata, what do you think about this? We could start of with a small subset of flake8 checks and increase the number over time. @c4xuxo, are you willing to implement that too?

@tataganesh
Copy link
Member

We could start of with a small subset of flake8 checks and increase the number over time.

This is a good idea. That way, code committed will always comply to flake8.

@pietermarsman
Copy link
Member

@c4xuxo do you have time to work on this?

I did some research on flake8 and I think we can start with a small subset that indicate bugs or errors:

  • E9* - possible runtime errors
  • F* - pyflake errors (e.g. undefined name)
  • W*. - warnings (but ignore W5* because these are ignored by default)

So an example .flake8 could be:

[flake8]
exclude = .git,__pycache__,old,build,dist,cmaprsrc,docs,samples
select = C,E9,F,W
ignore = W50

@pietermarsman
Copy link
Member

Hi @c4xuxo, I think it is a good idea to have some coding standards! I also have some requests. Do you have time to work on that?

@pietermarsman
Copy link
Member

@ganeshtata, this seems to be an inactive PR. I think we should close PR's without active contributors.

@c4xuxo
Copy link
Author

c4xuxo commented Aug 24, 2019

@pietermarsman eventually I will find sometime.... lets see...
It also can be close no a problem from my side.

@pietermarsman
Copy link
Member

I'm closing this PR because it has to many merge conflicts.

I still a big fan of using pep8 guidelines or flake8 enforcement, but I think we can better start over than use this PR. Feel free to reopen if you think it will cost you less time using this existing PR.

I've created an issue so that we will not forget about it: #312

@pietermarsman pietermarsman mentioned this pull request Dec 29, 2019
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants