-
Notifications
You must be signed in to change notification settings - Fork 0
Fix vscode integration #52
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
Conversation
Reviewer's Guide by SourceryThis pull request fixes integration with vscode Test Explorer by changing the CellPath format to use subscript notation (path/to/notebook.ipynb[Celln]) and patching pytest functions to handle the new format. It also updates tests to reflect the change in CellPath format. Sequence diagram for test execution within a cellsequenceDiagram
participant Cell
participant _ParsedNotebook
participant dummy_module
participant linecache
Cell->>_ParsedNotebook: _getobj()
activate Cell
Cell->>_ParsedNotebook: Get cell source
_ParsedNotebook-->>Cell: cell source
Cell->>dummy_module: exec(cellsabove, dummy_module.__dict__)
Cell->>dummy_module: exec(testcell, dummy_module.__dict__)
Cell->>linecache: cache[cell_filename] = (..., testcell_source, ...)
Cell-->>pytest: dummy_module
deactivate Cell
Updated class diagram for CellPathclassDiagram
class CellPath {
-notebook: Path
-cell: str
+__eq__(other: object) bool
+__hash__() int
+exists(*args: Any, **kwargs: Any) bool
+relative_to(other: PathLike, *args: Any, **kwargs: Any) Self
+notebook() Path
+cell() str
+is_cellpath(path: str) bool
+get_notebookpath(path: str) Path
+get_cellid(path: str) int
+to_nodeid(path: str) str
+patch_pytest_absolutepath() dict[tuple[ModuleType, str], FunctionType]
}
Path <|-- CellPath
class Path {
}
CellPath .. Path : notebook
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @MusicalNinjaDad - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding a helper function to encapsulate the logic for patching pytest functions, making the code more modular and readable.
- The
pytest_load_initial_conftests
hook modifies command-line arguments; ensure this doesn't interfere with other plugins or pytest's argument parsing logic.
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟡 Review instructions: 4 issues found
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
return f"{CELL_PREFIX}{self.get_cellid(str(self))}" | ||
|
||
@classmethod | ||
def is_cellpath(cls, path: str) -> bool: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (bug_risk): Assess robustness of is_cellpath string splitting logic.
The current implementation relies on splitting the string based on the '[Cell' substring. Consider edge cases such as unexpected additional '[' characters or variations of file names that might inadvertently fulfill the condition. If these formats are strictly controlled, the code is acceptable; if not, enhanced validation might be warranted.
Suggested implementation:
import re
# (existing imports)
@classmethod
def is_cellpath(cls, path: str) -> bool:
"""Determine whether a str is a valid representation of our pseudo-path."""
pattern = r'^(?P<notebook>.+\.ipynb)\[' + re.escape(CELL_PREFIX) + r'(?P<cellid>\d+)\]$'
return re.fullmatch(pattern, path) is not None
If CELL_PREFIX is subject to change or defined elsewhere with different values, ensure that its usage with re.escape is valid across the codebase.
Summary by Sourcery
Fixes VS Code integration by changing the CellPath format to use subscript notation (path/to/notebook.ipynb[Celln]) and patching pytest to handle this new format.
Bug Fixes:
Enhancements:
path/to/notebook.ipynb[Celln]
.Tests: