💚 black CI by GitHub Actions #30

[tkoyama010]

🆕 black CI by GitHub Actions for issue #30
In this PR. We need to run GitHub Actions.

[codecov]

Codecov Report

Merging #38 into master will not change coverage.
The diff coverage is 98.64%.

@@           Coverage Diff           @@
##           master      #38   +/-   ##
=======================================
  Coverage   94.76%   94.76%           
=======================================
  Files           2        2           
  Lines         420      420           
=======================================
  Hits          398      398           
  Misses         22       22           

[GuillaumeFavelier]

Thanks for taking care of this @tkoyama010 ! Now I have a better idea how black can be integrated :)

Maybe a stupid question but how do we know if it works before merging into master though? 😅

[tkoyama010]

@GuillaumeFavelier That is a good question. I don't know it neither 🤔 I plan to do some research 🔍

[tkoyama010]

My guess is that it is because this branch is in my fork repository. In other words, you could create a test branch in pyvistaqt and check it there. If you could make a branch, I will reconfigure this PR to merge into it.
I could run it in my fork repository.
tkoyama010#1

[tkoyama010]

This branch is passed in my fork repository 👍

[GuillaumeFavelier]

Could you explain what happened? I can see that the github action passed on your patch-1 branch in your repo but I thought that it would be triggered only on master.

Does it mean that there will be a commit "reformat by black" on each PR from now on?

Sorry for the noise, I'm just not used to black.

[tkoyama010]

@GuillaumeFavelier Sorry, my explanation was not enough. This CI is just to check if the PR source code follows black. I pushed the new commit "reformat by black" by myself to fix the CI error 🙇‍♂️

[GuillaumeFavelier]

Many thanks for clarifying 👍 I get it now :)

You have my +1 @tkoyama010, what about the other @pyvista/developers ?

[larsoner]

Otherwise looks fine to me

[tkoyama010]

set enable all check option ✔️

[tkoyama010]

Please wait while CI pass in tkoyama010#1 .

[tkoyama010]

If we use all enable option we have to solve this CI error.
https://github.com/tkoyama010/pyvistaqt/pull/1/checks?check_run_id=881454932

[GuillaumeFavelier]

We can still make pylint happy in a follow up PR if necessary 👍

[tkoyama010]

We can still make pylint happy in a follow up PR if necessary 👍

OK. Let's make pylint PR next time. Could you merge this PR first?

[GuillaumeFavelier]

Could you merge this PR first?

Sure, I would just like to wait a little for @akaszynski or @banesullivan opinion before merging if you don't mind.

[banesullivan]

LGTM! I'm a fan of this

I'd like to see more things used like pylint, isort, and flake8, but we can address those in follow up PRs

@tkoyama010, can you add a makefile (or tox) to do this testing locally?

[akaszynski]

I'm also an favor of this. We all have a coding style and it would be great to have a truly uniform one across the project.

I'm going to miss the single quotes though...

[akaszynski]

Would also like a makefile that runs black. @tkoyama010, think you could add that?

[banesullivan]

I'm going to miss the single quotes though...

Same. ' >> ". But with Black, it doesn't even matter. Code how you like, run Black and it should fix em. We can also switch it to prefer single quotes, but they have pretty good reasoning for double quotes

Would also like a makefile that runs black.

Tox is also another way of doing this, but @tkoyama010, do whatever is most simple

[akaszynski]

Why settle on double quotes? They anticipate apostrophes in English text. They match the docstring standard described in PEP 257. An empty string in double quotes ("") is impossible to confuse with a one double-quote regardless of fonts and syntax highlighting used. On top of this, double quotes for strings are consistent with C which Python interacts a lot with.

On certain keyboard layouts like US English, typing single quotes is a bit easier than double quotes. The latter requires use of the Shift key. My recommendation here is to keep using whatever is faster to type and let Black handle the transformation.

As I'm working with C more on my "real job", it's a bit annoying to switch between the two, so I'll agree that there's value with that.

Tox is indeed another option as well. I like how it installs the testing env, etc.

[tkoyama010]

@tkoyama010, can you add a makefile (or tox) to do this testing locally?

+1 I'll add this.

[tkoyama010]

Does anyone know what the CI error means?

[tkoyama010]

The CI error is an HTTP error, so it seems to be resolved by retrying.

[tkoyama010]

It is ready for merge 👍

[akaszynski]

@banesullivan, I think this one needs your approval. I'm happy with this.

[banesullivan]

LGTM! Thanks for moving us forward on this, @tkoyama010!

[tkoyama010]

@GuillaumeFavelier @larsoner @banesullivan @akaszynski Thank you. Now that we have reached an agreement, could you please merge it?