fix: Add support for pathlib in notebook field#3636
Conversation
📝 WalkthroughWalkthroughThe Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant notebook
Caller->>notebook: Call notebook(path, ...)
alt path is Path object
notebook->>notebook: Convert path to string
end
notebook->>notebook: Format path with wildcards and params
notebook-->>Caller: Return result
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/snakemake/notebook.py (1)
281-282: Prefer os.fspath for converting path-like objects.Instead of restricting the conversion to
pathlib.Path, useos.fspath(path)so that anyos.PathLike(and strings) are handled uniformly:- if isinstance(path, Path): - path = str(path) + path = os.fspath(path)Also, please add a unit test to verify that passing a
pathlib.Path(and otherPathLike) tonotebook()works as expected.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/snakemake/notebook.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.py`: Do not try to improve formatting. Do not suggest type annotations for functions that are defined inside of functions or methods. Do not suggest type annotation of the `s...
**/*.py: Do not try to improve formatting.
Do not suggest type annotations for functions that are defined inside of functions or methods.
Do not suggest type annotation of theselfargument of methods.
Do not suggest type annotation of theclsargument of classmethods.
Do not suggest return type annotation if a function or method does not contain areturnstatement.
src/snakemake/notebook.py
|
@staadecker: Are you still interested in finishing this PR? I could help you with test design etc. |
notebook fieldnotebook field
|
I'm afraid I don't quite have the time. Feel free to close the PR if needed. |
PR snakemake#3636 added support for Pathlike `notebook` specifications but @staadecker (the original author of that PR) did not provide a matching test. This commit add such a test based on @staadecker's example. I have verified that this test * fails (with the expected `TypeError`: ``` TypeError in file "</path/to/testdir>/Snakefile", line 7: expected str, got PosixPath ``` ) at commit 8bd1df9 (v9.6.0; parent of @staadecker's original PR snakemake#3636), * passes after applying the changes of commit 4445bb8 (@staadecker's original PR snakemake#3636), * still fails (with the same expected `TypeError`, see above) at commit f8b8620 (v.913.4; current `main`), and * still passes at commit 37a017d (after merging the current `main` (see above) on top of @staadecker's original PR snakemake#3636).
|
@staadecker: I added a test on top of your work. Since I don't have write permission to your branch, I opened a new PR (#3811). This one should automatically be closed once mine gets merged. Thank you for your contribution. |
PR snakemake#3636 added support for Pathlike `notebook` specifications but @staadecker (the original author of that PR) did not provide a matching test. This commit add such a test based on @staadecker's example. I have verified that this test * fails (with the expected `TypeError`: ``` TypeError in file "</path/to/testdir>/Snakefile", line 7: expected str, got PosixPath ``` ) at commit 8bd1df9 (v9.6.0; parent of @staadecker's original PR snakemake#3636), * passes after applying the changes of commit 4445bb8 (@staadecker's original PR snakemake#3636), * still fails (with the same expected `TypeError`, see above) at commit f8b8620 (v.913.4; current `main`), and * still passes at commit 37a017d (after merging the current `main` (see above) on top of @staadecker's original PR snakemake#3636).
PR snakemake#3636 added support for Pathlike `notebook` specifications but @staadecker (the original author of that PR) did not provide a matching test. This commit add such a test based on @staadecker's example. I have verified that this test * fails (with the expected `TypeError`: ``` TypeError in file "</path/to/testdir>/Snakefile", line 7: expected str, got PosixPath ``` ) at commit 8bd1df9 (v9.6.0; parent of @staadecker's original PR snakemake#3636), * passes after applying the changes of commit 4445bb8 (@staadecker's original PR snakemake#3636), * still fails (with the same expected `TypeError`, see above) at commit 395a5e6 (current `main`), and * still passes at commit 756656d (after merging the current `main` (see above) on top of @staadecker's original PR snakemake#3636).
PR #3636 added support for Pathlike `notebook` specifications but @staadecker (the original author of that PR) did not provide a matching test. This PR superseeds @staadecker's original PR #3636 by merging in current `main` and adding a commit that adds such a test based on @staadecker's example. I have verified that this test * fails (with the expected `TypeError`: ``` TypeError in file "</path/to/testdir>/Snakefile", line 7: expected str, got PosixPath ``` ) at commit 8bd1df9 (v9.6.0; parent of @staadecker's original PR #3636), * passes after applying the changes of commit 4445bb8 (@staadecker's original PR #3636), * still fails (with the same expected `TypeError`, see above) at commit 395a5e6 (current `main`), and * still passes at commit 756656d merging the current `main` (see above) on top of @staadecker's original PR #3636). --- closes #3636 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Ensure pathlike notebook resources are converted to strings before processing to avoid inconsistent handling. * **Tests** * Added test coverage verifying Jupyter notebook pathlike object support with Conda deployment. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Martin <machstg@gmail.com>
PR snakemake#3636 added support for Pathlike `notebook` specifications but @staadecker (the original author of that PR) did not provide a matching test. This PR superseeds @staadecker's original PR snakemake#3636 by merging in current `main` and adding a commit that adds such a test based on @staadecker's example. I have verified that this test * fails (with the expected `TypeError`: ``` TypeError in file "</path/to/testdir>/Snakefile", line 7: expected str, got PosixPath ``` ) at commit 8bd1df9 (v9.6.0; parent of @staadecker's original PR snakemake#3636), * passes after applying the changes of commit 4445bb8 (@staadecker's original PR snakemake#3636), * still fails (with the same expected `TypeError`, see above) at commit 395a5e6 (current `main`), and * still passes at commit 756656d merging the current `main` (see above) on top of @staadecker's original PR snakemake#3636). --- closes snakemake#3636 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Ensure pathlike notebook resources are converted to strings before processing to avoid inconsistent handling. * **Tests** * Added test coverage verifying Jupyter notebook pathlike object support with Conda deployment. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Martin <machstg@gmail.com>
The following:
Produces an error:
This PR adds support for
Pathobjects in thenotebookfield. I did not have time (to learn how) to make a test case.Summary by CodeRabbit