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

PEP8 conformance #72

Closed
yadudoc opened this Issue Jan 19, 2018 · 9 comments

Comments

Projects
None yet
3 participants
@yadudoc
Contributor

yadudoc commented Jan 19, 2018

Running flake8 on parsl throws 4582 errors. This is not ideal.
This article goes over some tools to auto fix code conformance issues.

  • Decide exceptions to conformance rules if any (line length, extra spaces ?)
  • Add config file with exceptions
  • Run auto fix tools and update changes
  • Add flake8 conformance as a test to travis, so as to fail a code does not conform to guidelines.

I'm tagging this for 0.4.0, but since the release deadline is very close might defer for a later point release since the code should be functionally identical.

@yadudoc yadudoc added this to the Parsl-0.4.0 milestone Jan 19, 2018

@yadudoc yadudoc self-assigned this Jan 19, 2018

@annawoodard

This comment has been minimized.

Collaborator

annawoodard commented Jan 19, 2018

I have autopep8 and yapf hooked into my vimrc so that issues are automatically complained about as I code, and I can automatically fix them. We should include instructions for doing so for vim and emacs. I'll add that to #65 TODOs. I can do the vim one.

@annawoodard

This comment has been minimized.

Collaborator

annawoodard commented Jan 19, 2018

(I edited the above comment with an additional link)

@benhg

This comment has been minimized.

Collaborator

benhg commented Jan 21, 2018

I have that setup in emacs and can write instructions for it

yadudoc added a commit that referenced this issue Jan 28, 2018

yadudoc added a commit that referenced this issue Jan 29, 2018

yadudoc added a commit that referenced this issue Jan 29, 2018

PEP8 Conformance fixes. Fixing E211,E221,E222,E225,E228,E231,E251. #72
    15    E211 whitespace before '('
    167   E221 multiple spaces before operator
    3     E222 multiple spaces after operator
    15    E225 missing whitespace around operator
    2     E228 missing whitespace around modulo operator
    99    E231 missing whitespace after ':'
    324   E251 unexpected spaces around keyword / parameter equals

yadudoc added a commit that referenced this issue Jan 29, 2018

yadudoc added a commit that referenced this issue Jan 29, 2018

yadudoc added a commit that referenced this issue Jan 29, 2018

yadudoc added a commit that referenced this issue Jan 29, 2018

yadudoc added a commit that referenced this issue Jan 29, 2018

yadudoc added a commit that referenced this issue Jan 29, 2018

yadudoc added a commit that referenced this issue Jan 29, 2018

yadudoc added a commit that referenced this issue Jan 29, 2018

yadudoc added a commit that referenced this issue Jan 29, 2018

@yadudoc

This comment has been minimized.

Contributor

yadudoc commented Jan 29, 2018

Let call this done until we have plans for improvements to make ?

@annawoodard

This comment has been minimized.

Collaborator

annawoodard commented Jan 29, 2018

It looks like flake8 is still complaining about a few things, since the last build is failing?

@annawoodard

This comment has been minimized.

Collaborator

annawoodard commented Jan 29, 2018

Also minor comment here: why exclude tests in the flake8 config? For me, the benefits of linting apply there too.

@yadudoc

This comment has been minimized.

Contributor

yadudoc commented Jan 29, 2018

Hmm.. build looks green for me now (since last 5 hrs)? https://travis-ci.org/Parsl/parsl/builds
I need to go back and fix the tests. That was not intentional.

@annawoodard

This comment has been minimized.

Collaborator

annawoodard commented Jan 29, 2018

Ah sorry 🤦‍♀️ I just saw fdce2e7 and didn't look at the most recent. I agree then, with tests added to the config, this issue is good to go. 👏 👏 👏 🎉 🎉 🎉

yadudoc added a commit that referenced this issue Jan 30, 2018

@yadudoc

This comment has been minimized.

Contributor

yadudoc commented Jan 30, 2018

Tests are added, and travis is showing all green. Closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment