Pylint support. Replace pass with type for handling magics

[girip11]

This is required to support pylint. Using pass causes pylint to raise unnecessary pass warnings. Instead if we use builtin function type we are clean of any flake8 and pylint warnings.

[girip11]

Pending

• Pylint tests
• Confirm we don't break any of the already supported tools
• Add test case for black with low line length
• Update docs

[girip11]

Input

Though %%time is cell magic, having it closing the function/code seems to look normal than having two lines separating the magic which makes the cell look odd.

%%time
def func():
     pass

Current behavior

After black formats current code, we have two lines separating the cell magic and python function which looks odd.
pass # nbqa <token> is used as the replacement string in the temp python file.

%%time


def func():
     pass

New behavior

Cell magics will become comments. After black formats current code, comments are not moved two lines apart. # CELL_MAGIC %%time <token> is used as the replacement string in the temp python file

%%time
def func():
     pass

[MarcoGorelli]

Nice! This would be a breaking change for nbqa black but I think it's OK, this is better, and also this library is very young.

Did __import__( not work?

[girip11]

Did __import__( not work?

• I used __import__. __import__ template alone takes up around 60-65 characters. When I tried solving Don't ignore in-line magic (when possible) #279, I ran in to the risk of line length exceeding 90 chars sometimes using this __import__ template. I think @MarcoGorelli you already captured this and raised concern around this.
• Once this happens black splits the statement into multiple lines which makes replacing the original statement a bit harder.
• In case of linters, we would get line length exceeded warnings.
• So I resorted back to using builtin function type as it takes just 4 characters.

But we still have those issues of black splitting these replacement statements in to multiple lines and linter warnings. But the probability of running in to those is low compared to __import__.

[MarcoGorelli]

I see, thanks for explaining.

Would it be possible to also put the magic in a comment, so that the only valid Python code here is type() and so there's no risk of black splitting it into multiple lines?

[girip11]

Would it be possible to also put the magic in a comment, so that the only valid Python code here is type() and so there's no risk of black splitting it into multiple lines?

Sorry I didn't get you. Could you elaborate on this?

[MarcoGorelli]

You currently have

'type(""" {magic} """)  # {token}'

but I think black could potentially split it into

'type(
    """ {magic} """
)  # {token}'

How about something like

'type()  # {magic} {token}'

? I don't think there's any scenario under which black would split this

[girip11]

'type() # {magic} {token}'

Good point. But we can't have just type() without any argument. mypy would raise error. Just checked. And the moment we add type(1) black is going to split the lines.

I am inclining towards handling multi line split in replace_magics with the help of token in the string. We can take those changes in a separate PR, since this kind of scenario is rare.

[MarcoGorelli]

Good point. But we can't have just type() without any argument. mypy would raise error.

🤣 it feels like flattening a carpet that's too big for the floor

[MarcoGorelli]

@girip11 how about 'int() # {magic} {token}'?

[girip11]

@girip11 how about 'int() # {magic} {token}'?

Thanks @MarcoGorelli. This would work with currently supported/tested tools.

Meanwhile I was able to come up with a solution using regex matches. Even if modifications done those temporary placeholder lines we would still be able to restore the original line magics as long as the token is untouched. If we hit any issues with this approach i think we can tryout your int() suggestion.

Approach

I used type() and I used the format suggested by @MarcoGorelli 'type({token}) # {magic} {token}' but truncated the {magic} to first 10 characters.

By using the token in the beginning and at the end of the code, we would be able to replace the entire code across multiple lines using regex match. I will raise next iteration of the PR with those changes incorporated. @MarcoGorelli and @s-weigand feel free to take a look at those changes though I have kept the PR still in the draft mode.

With tests added for pylint, I will make the PR ready for review.

[codecov-io]

Codecov Report

Merging #302 into master will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master      #302   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            9        10    +1     
  Lines          418       469   +51     
=========================================
+ Hits           418       469   +51     

Impacted Files	Coverage Δ
nbqa/__main__.py	100.00% <100.00%> (ø)
nbqa/handle_magics.py	100.00% <100.00%> (ø)
nbqa/notebook_info.py	100.00% <100.00%> (ø)
nbqa/replace_source.py	100.00% <100.00%> (ø)
nbqa/save_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 8c37f56...0a9d37a. Read the comment docs.

[MarcoGorelli]

By using the token in the beginning and at the end of the code, we would be able to replace the entire code across multiple lines using regex match.

Clever! I'm normally scared of regexes, but this seems like a safe/legitimate use of them!

[MarcoGorelli]

Nice!

Generally, this looks really good

Regarding

I will raise next iteration of the PR with those changes incorporated.

do you mean this would be a separate PR? Because currently, if I run nbqa black tests/data/notebook_with_indented_magics.ipynb --line-length=15 --nbqa-mutate then the second cell becomes

from random import (
    randint,
)

if (
    randint(
        5, 10
    )
    > 8
):
    type(
        0xD0136BEE
    )  # %time prin 0xD0136BEE

TBH I'd rather not knowingly introduce a regression...I don't know how common it is to run black with a short line-length, but still, I guess there may be legitimate uses for it and I'd rather not break such notebooks with mysterious hashes 😄

[MarcoGorelli]

@girip11 if I apply the following diff

diff --git a/nbqa/handle_magics.py b/nbqa/handle_magics.py
index e0ad7c6..e5ee464 100644
--- a/nbqa/handle_magics.py
+++ b/nbqa/handle_magics.py
@@ -73,8 +73,8 @@ class MagicHandler(ABC):
     # `{token}` is not used as `String` because with different formatters (e.g: yapf)
     # we would run in to formatting issues like single quotes formatted
     # to double quotes or vice versa. `{token}` is used as hexadecimal number.
-    _MAGIC_TEMPLATE: str = "type({token})  # {magic:10.10} {token}"
-    _MAGIC_REGEX_TEMPLATE: str = r"type\({token}\).*{token}"
+    _MAGIC_TEMPLATE: str = "int()  # {token} {magic:10.10} {token}"
+    _MAGIC_REGEX_TEMPLATE: str = r"int\(\)  # {token}.*{token}"
 
     def replace_magic(self, ipython_magic: str) -> MagicSubstitution:
         """

then all the current tests still pass, and nbqa black tests/data/notebook_with_indented_magics.ipynb --line-length=15 --nbqa-mutate keeps the notebook in a good state.

Would this work for you? Is there an advantage of type (or disadvantage or int()) which I'm missing?

(we might still get a false positive from flake8 like this, but IMO a false positive from a static analysis tool is far less undesirable than modified output from a formatter, which I fear would lose people's confidence in our tool)

[girip11]

TBH I'd rather not knowingly introduce a regression...I don't know how common it is to run black with a short line-length, but still, I guess there may be legitimate uses for it and I'd rather not break such notebooks with mysterious hashes.

Good catch. There are two ways to go about the line length issue.

1. Fix existing regex to handle spaces r"type\s*\(\s*{token}\s*\).*{token}"
2. Use the int() solution suggested that you suggested.

I was using type() just because i thought it was safer compared to int(). What if some tool throws warnings on its usage?
We could also use int({token}) regex with spaces handled. So the reported black formatting issue would go away.

What are your thoughts on this?

[girip11]

I will raise next iteration of the PR with those changes incorporated.

I have made all the changes. This PR is ready for review. Only pylint usage in docs needs to be updated.

[girip11]

(we might still get a false positive from flake8 like this, but IMO a false positive from a static analysis tool is far less undesirable than modified output from a formatter, which I fear would lose people's confidence in our tool)

I agree with you on this. I just thought who would use line length to be as less as 50. So I just ignored space matches in the regex. Fixing the regex would solve the formatting issue.

[MarcoGorelli]

Fixing the regex would solve the formatting issue.

Sure, I'm happy to go with that! 👍

[girip11]

I will add this short line length as a test case for black in order to avoid regressions of this sort in the future.

[MarcoGorelli]

Cool, this one works! Thanks, excellent work! I think we're ready for a 0.3.0 release (as this is a new feature), I'll just try to get #305 in as well

[MarcoGorelli]

I'll have another quick look later to double-check but I'm pretty sure this is good to go, thanks!

[MarcoGorelli]

Awesome, look good!!