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

Return NothingChanged if non-Python cell magic is detected, to avoid tokenize error #2630

Merged
merged 13 commits into from Nov 29, 2021
Merged

Return NothingChanged if non-Python cell magic is detected, to avoid tokenize error #2630

merged 13 commits into from Nov 29, 2021

Conversation

danielsparing
Copy link
Contributor

@danielsparing danielsparing commented Nov 19, 2021

Description

Fixes #2627 , a non-Python cell magic such as %%writeline can legitimately contain "incorrect" indentation, however this causes tokenize-rt to return an error. To avoid this, validate_cell should early detect cell magics (just like it detects TransformerManager transformations).

Test added too, in the shape of a "badly indented" %%writefile within test_non_python_magics.

Checklist - did you ...

  • Add a CHANGELOG entry if necessary?
  • Add / update tests if necessary?
  • Add new / update outdated documentation?

Daniel Sparing added 6 commits November 18, 2021 21:10
Let `validate_cell()` also catch non-Python cell magics, as the bodies of these might contain invalid indentation (such as `%%writefile`) and this could cause TransformerManager to break.
this should be a valid arbitrary writefile, but before it failed via the tokenizer.
Copy link
Contributor

@MarcoGorelli MarcoGorelli left a comment

Choose a reason for hiding this comment

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

Thanks @danielsparing for this PR!

I'm not a black maintainer, but I did make the Jupyter PR, so am leaving a couple of comments:

1 - does this mean that lines

if cell_magic_finder.cell_magic.name in NON_PYTHON_CELL_MAGICS:
raise NothingChanged

can be removed?

  1. Perhaps this should be done the other way round? I.e. rather than checking if the magic name is in a blocklist, and skipping the cell if that's the case, black should check if the magic name is in an allow list, and only format the cell if it is. Thoughts on this?

Daniel Sparing added 2 commits November 19, 2021 15:33
and remove later unnecessary check
as we are now allowlisting python cell magics and %t would be a user-defined custom one
@danielsparing
Copy link
Contributor Author

@MarcoGorelli Thanks for the quick review!

I agree that the allowlist is cleaner. This also necessitates something I should have done earlier, to exactly check for the first word in the cell for the cell magic.

This does mean that I needed to remove the test with %%t and make it a %%timeit as it is a custom magic, so shouldn't be in the allowlist.

I also removed the lines you mentioned.

What do you think?

Copy link
Contributor

@MarcoGorelli MarcoGorelli left a comment

Choose a reason for hiding this comment

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

Thanks for updating @danielsparing !

Generally looks good to me, tagging @JelleZijlstra and @ichard26 (who reviewed the original PR extensively) for further comments

src/black/handle_ipynb_magics.py Outdated Show resolved Hide resolved
CHANGES.md Outdated

### _Black_

- Fixed non-Python cell magics sometimes failing due to indentation (#2630)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this can be made more explicit - can you make it clearer what the behaviour was before, and what it is now, so it's clear that black is now intentionally more timid about processing cell magics?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

addressed in 1e8a0b1

Copy link
Contributor

@MarcoGorelli MarcoGorelli left a comment

Choose a reason for hiding this comment

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

Awesome - just got a minor comment left, will handover to the black maintainers now

src/black/handle_ipynb_magics.py Outdated Show resolved Hide resolved
as `Black` does not support Python2 anymore
danielsparing pushed a commit to GoogleCloudPlatform/asl-ml-immersion that referenced this pull request Nov 24, 2021
src/black/__init__.py Outdated Show resolved Hide resolved
Co-authored-by: Marco Edward Gorelli <marcogorelli@protonmail.com>
@JelleZijlstra JelleZijlstra merged commit a066a2b into psf:main Nov 29, 2021
@ichard26
Copy link
Collaborator

Thank you so much for your contribution! This project is only possible by contributions like these 🖤. Seriously, the core team barely has any experience with Jupyter notebooks so we rely on third-party contributions to keep this feature of Black alive and well. You're awesome, @danielsparing - congrats on your first PR to psf/black 🎉

I'm also going to take this moment to thank the defacto core dev on black's jupyter support: @MarcoGorelli!

Y'all are great :)

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.

Jupyter cell magics are not completely ignored
4 participants