Fix vscode integration #54
Conversation
… mixin to CellPath.PytestItemMixin
This reverts commit 693b783.
Reviewer's Guide by SourceryThis pull request fixes the vscode integration by changing the CellPath format to Sequence diagram for pytest Cell collectionsequenceDiagram
participant pytest
participant Notebook
participant Cell
participant ParsedNotebook
pytest->>Notebook: pytest_collect_file(file_path, parent)
Notebook->>ParsedNotebook: _ParsedNotebook(self.path)
loop for each testcellid in parsed.muggled_testcells.ids()
Notebook->>Cell: Cell.from_parent(parent, name, nodeid, path)
Cell-->>Notebook: cell
Notebook->>Cell: cell.stash[ipynb2_notebook] = parsed
Notebook->>Cell: cell.stash[ipynb2_cellid] = testcellid
Notebook-->>pytest: yield cell
end
Sequence diagram for pytest session start and finishsequenceDiagram
participant pytest
participant CellPath
pytest->>CellPath: pytest_sessionstart(session)
CellPath->>CellPath: patch_pytest_absolutepath()
CellPath-->>pytest: Monkeypatches
pytest->>CellPath: pytest_sessionfinish(session, exitstatus)
CellPath->>CellPath: Revert Monkeypatches
CellPath-->>pytest: None
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
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #54 +/- ##
============================================
- Coverage 100.00% 71.91% -28.09%
============================================
Files 3 4 +1
Lines 345 349 +4
Branches 28 29 +1
============================================
- Hits 345 251 -94
- Misses 0 97 +97
- Partials 0 1 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @MusicalNinjaDad - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding a helper function to construct
ExampleDirSpecinstances in tests to reduce duplication. - The new
CellPathformat is more standard and readable.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟡 Review instructions: 3 issues found
- 🟡 Testing: 2 issues found
- 🟡 Complexity: 1 issue found
- 🟢 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.
|
|
||
| ### Fixed | ||
|
|
||
| - Integration with vscode Test Explorer [#54][pr-54] |
There was a problem hiding this comment.
nitpick (typo): Typo: "vscode" should be "VS Code"
Consider using the official capitalization "VS Code" for consistency.
| - Integration with vscode Test Explorer [#54][pr-54] | |
| - Integration with VS Code Test Explorer [#54][pr-54] |
| item_type = type(item) | ||
| type_with_mixin = types.new_class(item_type.__name__, (CellPath.PytestItemMixin, item_type)) | ||
| item.__class__ = type_with_mixin | ||
| yield item |
There was a problem hiding this comment.
issue (complexity): Consider using explicit inheritance with a wrapper factory instead of dynamically creating a new class in collect() to reduce cognitive overhead, and move monkey-patching logic to its own module with inline comments explaining why the patching is required to reduce indirection and complexity while preserving functionality
The general approach is good, but the dynamic re-blessing of items in collect() adds significant cognitive overhead. Consider extracting the wrapping into a small helper and using explicit inheritance instead of dynamically creating a new class.
For example, introduce a wrapper factory and refactor collect() like so:
def wrap_pytest_item(item: pytest.Function) -> pytest.Function:
# Explicitly define a subclass using our mixin and the item's original type
class WrappedItem(CellPath.PytestItemMixin, type(item)):
pass
item.__class__ = WrappedItem
return itemThen, update collect() to delegate the wrapping:
def collect(self) -> Generator[pytest.Function, None, None]:
for item in super().collect():
yield wrap_pytest_item(item)This makes the intent clear, isolates the dynamic modification in a small helper, and reduces in-line complexity while preserving functionality.
Additionally, consider:
- Moving monkey-patching logic (i.e.
patch_pytest_absolutepath) to its own module or function. - Adding concise inline comments about why the patching is required.
These changes keep the functionality intact while reducing the indirection and complexity.
| """The cell specifier (e.g. "Cell0").""" | ||
| return f"{CELL_PREFIX}{self.get_cellid(str(self))}" | ||
|
|
||
| @classmethod |
There was a problem hiding this comment.
suggestion (review_instructions): The get_notebookpath and get_cellid methods could use EAFP instead of relying on string parsing
These methods parse strings assuming a specific format. Consider using try/except to handle cases where the string doesn't match the expected format, which would follow the EAFP principle.
Review instructions:
Path patterns: **/*.py, **/*.ipynb
Instructions:
Prefer EAFP to LBYL in most cases
Summary by Sourcery
Fixes integration with VS Code's test explorer 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]