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

💚 Add pycodestyle in CI #45

Merged
merged 37 commits into from
Aug 4, 2020
Merged

Conversation

tkoyama010
Copy link
Member

Let's add pycodestyle in CI !

@tkoyama010 tkoyama010 marked this pull request as draft July 31, 2020 14:18
@tkoyama010 tkoyama010 changed the title 💚 Add pycodestyle in CI 🚧 💚 Add pycodestyle in CI Jul 31, 2020
@codecov
Copy link

codecov bot commented Jul 31, 2020

Codecov Report

Merging #45 into master will decrease coverage by 0.22%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##           master      #45      +/-   ##
==========================================
- Coverage   94.88%   94.66%   -0.23%     
==========================================
  Files           5        5              
  Lines         430      431       +1     
==========================================
  Hits          408      408              
- Misses         22       23       +1     

@GuillaumeFavelier
Copy link
Contributor

I notice that the ouput of pycodestyle here is really close to the one in #46 (comment)

@GuillaumeFavelier
Copy link
Contributor

GuillaumeFavelier commented Jul 31, 2020

Most of it (E501) can be resolved by a convention for the number of characters by line and whether to allow whitespace after : or not (E203).

Finally, I think (E731) can be solved by using a local function instead of a lambda.

@tkoyama010
Copy link
Member Author

Most of it (E501) can be resolved by a convention for the number of characters by line and whether to allow whitespace after : or not (E203).

You are abosolutly right. But Black doesn't allow it and it is the most inflexible checker that we ultimately need to follow it. So let's add ignore option about it.

--max-line-length=88 --ignore="E203"

Finally, I think (E731) can be solved by using a local function instead of a lambda.

You are right thanks.

Copy link
Member Author

@tkoyama010 tkoyama010 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is self review 👀

.github/workflows/pythonpackage.yml Outdated Show resolved Hide resolved
pyvistaqt/plotting.py Outdated Show resolved Hide resolved
pyvistaqt/plotting.py Outdated Show resolved Hide resolved
pyvistaqt/plotting.py Outdated Show resolved Hide resolved
pyvistaqt/plotting.py Outdated Show resolved Hide resolved
pyvistaqt/plotting.py Outdated Show resolved Hide resolved
pyvistaqt/plotting.py Outdated Show resolved Hide resolved
pyvistaqt/plotting.py Outdated Show resolved Hide resolved
pyvistaqt/plotting.py Outdated Show resolved Hide resolved
pyvistaqt/plotting.py Outdated Show resolved Hide resolved
@tkoyama010
Copy link
Member Author

tkoyama010 commented Jul 31, 2020

Hmm. pycodestyle doesn't recognize ignore option 😕

@GuillaumeFavelier
Copy link
Contributor

Even without the "?

@tkoyama010
Copy link
Member Author

tkoyama010 commented Jul 31, 2020

Oh. It works! Thanks @GuillaumeFavelier 👍

@tkoyama010 tkoyama010 changed the title 🚧 💚 Add pycodestyle in CI 💚 Add pycodestyle in CI Jul 31, 2020
@tkoyama010 tkoyama010 marked this pull request as ready for review July 31, 2020 16:09
@tkoyama010
Copy link
Member Author

tkoyama010 commented Jul 31, 2020

Does someone know how to fix codecov error ?

Copy link
Contributor

@GuillaumeFavelier GuillaumeFavelier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

54db8db covered it for me locally but it's just the patch anyway, the overall coverage is still above the threshold so +1 for me.

Thanks @tkoyama010

@tkoyama010
Copy link
Member Author

tkoyama010 commented Aug 3, 2020

@GuillaumeFavelier Thanks😇
It is ready for merge.

@tkoyama010 tkoyama010 mentioned this pull request Aug 4, 2020
@GuillaumeFavelier GuillaumeFavelier merged commit 51922f0 into pyvista:master Aug 4, 2020
@GuillaumeFavelier
Copy link
Contributor

Thank you @tkoyama010 !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants