Repository navigation
refactor: one isnotebook() for the whole package - #916
Merged
Merged
Conversation
PyPlot.py and mobile/Vehicle.py each had their own notebook check, and they disagreed. Vehicle's looked up get_ipython in the module globals, where IPython does not put it, so its shell test never ran and it fell back on "ipykernel is loaded", true in a terminal IPython that imported it. Move the one in PyPlot.py, which handles JupyterLite, Colab and a Jupyter kernel, to tools/plot.py and use it from both. PyPlot.py and Vehicle.py still call it _isnotebook, so existing uses and tests that patch it are unchanged. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 4 |
| Duplication | 0 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PyPlot.pyandmobile/Vehicle.pyeach had a private_isnotebook(), and they disagreed.Vehicle's looked upget_ipythonwithglobals().get(...), but IPython puts that function inbuiltins, not in a module's globals, so its shell test never ran and it fell back on "ipykernelis insys.modules". That is also true in a terminal IPython that importedipykernel, whichPyPlotcorrectly treats as not a notebook.The version in
PyPlot.py, which handles JupyterLite, Google Colab and a Jupyter kernel, moves toroboticstoolbox/tools/plot.pyasisnotebook(). Both modules import it as_isnotebook, so the other imports of that name (PyPlot2,BaseRobot) and the tests that patch it are unchanged.Does this change numerical results or public behaviour? No numerical results change. The mobile module's notebook detection now matches the rest of the package: a terminal IPython that has
ipykernelloaded is no longer treated as a notebook there.spatialmath's
environment()andisnotebook()(rai-opensource/spatialmath-python#271, open) would replace this once released. This PR leaves RTB with one copy to replace instead of two.Tested: the full suite passes (2435 tests),
mypy2.4.1 is clean. New tests check thatPyPlotandVehicleuse the same function, that a Jupyter kernel is a notebook, and that a terminal IPython is not one even whenipykernelis loaded. They use mocks, not a real notebook frontend.This change was written with Claude Code (Claude Sonnet 5.5).
🤖 Generated with Claude Code