Repository navigation
fix(nblint): preserve multiline strings containing magic-like lines - #1445
Conversation
Signed-off-by: Mohammed Alshyakh <zzzzmmmm298@gmail.com>
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses an issue where the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request updates the syntax linter to avoid stripping IPython magics and shell escapes when they appear inside multiline strings, using Python's tokenize module. It also introduces a comprehensive suite of unit tests. The feedback suggests improving the robustness of the tokenization logic by handling Python 3.12+ f-string tokens, refining the TokenError exception handling to prevent false positives on mismatched parentheses, and adding a test case specifically for multiline f-strings.
Signed-off-by: Mohammed Alshyakh <zzzzmmmm298@gmail.com>
BichengWang
left a comment
There was a problem hiding this comment.
Reviewed 00a0ed0c2af816fc771bfaf677edb98610ed75f9. The ordinary-string and f-string regressions both fail against the base and pass here; all five syntax tests pass on Python 3.13, 3.14, and 3.15. The amended head also addresses the earlier f-string tokenization and TokenError feedback. I ran the 18 existing link-checker tests and compared syntax validation across all 154 notebooks against the base, with no new failures. Validation was offline; I did not execute notebook kernels or call the API. No blocking findings.
|
Thanks @mohmedmm, the fix is right. Verified: the #1443 reproducer passes, both regression tests fail on main and pass here, magics in the same cell as a multiline string are still neutralized, and a full nblint run over all notebooks is unchanged (93 warnings, 0 failures, same as main). Thanks @BichengWang for the cross-check on 3.13–3.15 matches what I see. One thing before merging: #1434 (also touching syntax.py) creates the same new file tools/test_nblint_syntax.py, so the two PRs conflict (add/add). Since #1434 is further along and wires that file into CI, could you rebase onto it once it lands and add your tests to the existing file? Everything else can stay as is. |
|
Sounds good, I will rebase and merge the tests into ools/test_nblint_syntax.py as soon as #1434 lands. |
Fixes #1443.
strip_ipython_syntax previously stripped lines starting with ! or % indiscriminately, breaking multiline strings whose interior lines contain magic-like commands or LaTeX comments. We track multiline string line intervals via standard library okenize so string contents are left untouched while real cell magics and shell escapes are still neutralized.