Repository navigation
fix(nblint): check Python syntax in timed cells - #1434
Conversation
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 improves the robustness of the notebook linting process by ensuring that Python code inside 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 notebook linter (nblint) to parse and validate the syntax of Python code cells that use the %%time cell magic, while continuing to skip other non-Python cell magics. It also introduces a comprehensive test suite in tools/test_nblint_syntax.py to verify this behavior. The feedback suggests optimizing the check for %%time by using src.split(maxsplit=1)[0] instead of src.split()[0] to avoid splitting the entire content of large notebook cells, which improves performance.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
Thanks, nice catch. Verified locally: 25/25 tests pass, the new tests fail against the current syntax.py, all 22 %%time cells in the repo pass as-is and each one is flagged if I append broken =, and a full nblint run over all notebooks is unchanged (0 failures, same warnings as main). LGTM. Same note as on #1432: tools/test_nblint_syntax.py isn't run by CI yet leaving that to @Giom-V. |
|
Added the syntax tests to the notebook workflow and enabled it for tool and workflow changes. All 25 syntax and link-checker tests pass locally. |
|
@Giom-V, the syntax tests now run in CI and all checks are green. Does the CI wiring look good to you? |
Giom-V
left a comment
There was a problem hiding this comment.
Thanks for the fix @BichengWang! I removed the unit tests and CI step from this PR so we don't have to maintain a separate unit test suite for internal helper scripts. Merging!
nblintskips every cell starting with%%, so%%time\nbroken =passes the syntax check even though its Python body cannot compile. There are 22%%timecells in the current notebooks.Check
%%timebodies using the existing IPython syntax normalization, which preserves the original cell line numbers. Keep other cell magics skipped because their bodies may contain non-Python content. Add offline regression tests and update the rule documentation.Validation:
python3 -m unittest tools.test_nblint_syntax tools.test_check_all_links -v— 25 tests pass; the new regressions fail on the original implementation.nblint_cli.pyrun over 154 notebooks — zero failures.broken =in memory.python3 tools/check_all_links.py tools/README.mdandgit diff --checkpass.No notebooks were executed or changed.