Add nbqa-yapf

[bdice]

Adds a new hook definition for yapf.

I'm a big fan of nbqa! It's truly an amazing tool. 💯

[MarcoGorelli]

Awesome, thanks! As well as this, it would be good add a simple test, and to show a little example in the docs - do you have time to do these too? No worries if not, I can do it next week

I'm a big fan of nbqa! It's truly an amazing tool. 100

Very kind of you, mind if I include this as a README testimonial?

[MarcoGorelli]

@all-contributors please add @bdice for ideas and code

[allcontributors]

@MarcoGorelli

I've put up a pull request to add @bdice! 🎉

[bdice]

I'm not sure how the --in-place modification flag interacts with --nbqa-mutate but this seemed to behave like I wanted (I had to provide --nbqa-mutate to make changes).

[bdice]

Awesome, thanks! As well as this, it would be good add a simple test, and to show a little example in the docs - do you have time to do these too? No worries if not, I can do it next week

I'm taking a look at tests and docs now. For testing, I'm going to just copy the tests for black and adjust the output to whatever yapf should produce.

I'm a big fan of nbqa! It's truly an amazing tool. 100

Very kind of you, mind if I include this as a README testimonial?

😄 Sure! Maybe a more descriptive testimonial would be: "nbqa is a clean, easy to use, and effective tool for notebook code style. Formatting and readability makes a huge difference when rendering notebooks in a project's documentation!" Feel free to use either statement. 🥳

[MarcoGorelli]

Awesome - yeah, I think that the test test_black_works_with_leading_comment would be a good one to copy, just make a short notebook in tests/data and check that nbqa yapf test/data/<your new notebook> --nbqa-diff looks as you'd expect it to

[bdice]

@MarcoGorelli Thanks again for the advice/help! I pushed changes to docs/tests. I copied the black tests, with a couple of exceptions:

• The test for failing when provided invalid syntax (test_black_works_with_literal_assignment) seems to do nothing with yapf. I don't know if this is a bug or an expected behavior of yapf, but I just didn't create that test for yapf.
• The test for successive runs (test_successive_runs_using_yapf) checks modification times. I couldn't get the modification times to change in the before/after on my system, so I altered the test for yapf to compare the raw file contents and ensure that the first pass modifies the notebook and the second pass does not.

[MarcoGorelli]

Hey @bdice ,

thanks, much appreciated that you looked into this and made extensive tests. This actually uncovers an issue. If we use two newlines between cell separators, then yapf wants to remove one. But if we use two newlines, then yapf might want to add one.

For example. if we have t.py as

# %%NBQA-CELL-SEP
import os


# %%NBQA-CELL-SEP
def hello_world():
    print('hello world')


hello_world()


# %%NBQA-CELL-SEP

then we will get

$ yapf t.py --diff
--- t.py	(original)
+++ t.py	(reformatted)
@@ -9,5 +9,4 @@
 
 hello_world()
 
-
 # %%NBQA-CELL-SEP

But, if instead we use single new lines as separators, e.g.

# %%NBQA-CELL-SEP
import os

# %%NBQA-CELL-SEP
def hello_world():
    print('hello world')


hello_world()

# %%NBQA-CELL-SEP

then we get

$ yapf t.py --diff
--- t.py	(original)
+++ t.py	(reformatted)
@@ -1,5 +1,6 @@
 # %%NBQA-CELL-SEP
 import os
+
 
 # %%NBQA-CELL-SEP
 def hello_world():

So...I'm a bit stuck about what to do here

[bdice]

@MarcoGorelli It looks like the statements hello_world() and # %%NBQA-CELL-SEP are being treated differently than def / class blocks. It looks like yapf prefers to leave at most one newline between statements like hello_world(), x = 5, or # My comment and two newlines between method/class definitions (and two newlines after the imports at the top).

Can you help me understand why this is an issue and what effect it has? Is it changing the number of trailing newlines in notebook cells in an unexpected way?

[MarcoGorelli]

Sure - the issue is that nbqa converts a notebook to a temporary Python file, with two newlines and a special comment to indicate separation between cells. The issue is that like this we end up in a situation in which yapf wants to either add or remove some newlines, and so nbqa yapf tests/data/notebook_for_testing.ipynb --in-place will always return "mutation detected".

The only workaround I can think of it to apply yapf to each cell one-by-one - this'd be quite a departure from how other tools are treated though, but I can't think of other options. Will this about it anyway

[bdice]

Aha, I see. Perhaps there is an option in the yapf configuration that would stop this? Perhaps BLANK_LINES_BETWEEN_TOP_LEVEL_IMPORTS_AND_VARIABLES=2?

[MarcoGorelli]

Great, yeah - if yapf is somewhat configurable then we could set that as the default in https://github.com/nbQA-dev/nbQA/blob/master/nbqa/config/default_config.toml , thanks!

[bdice]

@MarcoGorelli It seems like the option for configuring BLANK_LINES_BETWEEN_TOP_LEVEL_IMPORTS_AND_VARIABLES hasn't been released yet (0.31 is not yet out): google/yapf#347 (comment)

I'm not sure what to do about this, I will defer to your expertise if you have further ideas for how to handle it. Do we try the cell-by-cell approach or just add a warning to the docs section on yapf?

[MarcoGorelli]

Hey @bdice - I think my preference would be to either wait for the next release, or try the cell-by-cell approach (I'm about to start a new job though so don't really have the resources for the latter at the moment - I can review a PR if you put one together, else I hope to be able to get back to this later)

[MarcoGorelli]

Hey @bdice - looks like yapf 0.31.0 is out! So I think we can add this option and require this as the minimum version for yapf (similarly to how we require a minimum version for isort)

[bdice]

@MarcoGorelli Thanks! Unfortunately my guess was incorrect -- BLANK_LINES_BETWEEN_TOP_LEVEL_IMPORTS_AND_VARIABLES does not control that setting with blank lines around comments. I looked through the yapf settings and source, but I don't see how to change that behavior.

Likewise, I'm not sure that I have time to change to a "cell by cell" approach. I prefer using black and this PR is mostly intended to be a minor convenience for a project I contribute to that uses yapf (and to give something back to nbqa because the tool is wonderful).

In practice, the pre-commit hook I have set up with nbqa yapf appears to work as desired, and it's fine with us to always mutate the notebooks on commit.
https://github.com/glotzerlab/hoomd-examples/blob/3402c93a1842e5818d86dbc39a2b2d0912c84326/.pre-commit-config.yaml#L59-L69

Let me know what you'd like to do with this PR - I'm fine with whatever outcome you think is appropriate. Maybe we need to file a tracking issue with yapf?

[MarcoGorelli]

and to give something back to nbqa because the tool is wonderful

That's very kind of you 🙏

I'm also very short on time in this period (just started a new job) so let's close for now, we can always come back to this later

[MarcoGorelli]

Reopening as I have an idea for how to get this through without adding too much complexity

[bdice]

Reopening as I have an idea for how to get this through without adding too much complexity

Nice! Let me know if you’d like a helping hand or review.

[MarcoGorelli]

Sure, thanks! A review would be helpful if you have time :)

cc @girip11 @s-weigand if you have comments

[bdice]

@MarcoGorelli The changes look good to me. Thanks!

[girip11]

I will review today

[codecov-commenter]

Codecov Report

Merging #547 (f8abdf7) into master (1c57a52) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master      #547   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           14        14           
  Lines          781       797   +16     
  Branches       115       117    +2     
=========================================
+ Hits           781       797   +16     

Impacted Files	Coverage Δ
nbqa/__main__.py	100.00% <100.00%> (ø)
nbqa/output_parser.py	100.00% <100.00%> (ø)
nbqa/replace_source.py	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1c57a52...f8abdf7. Read the comment docs.

[MarcoGorelli]

merging for now as I'm pretty keen on making a new release before tomorrow, can always revert if it anyone has objections / causes bugs anywhere

[bdice]

@MarcoGorelli Thank you again for your guidance and help! 😄

[MarcoGorelli]

@bdice I forgot to mention, if you wanted to mention yapf in the README (alongside other tools), that'd be welcome!