-
Notifications
You must be signed in to change notification settings - Fork 186
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
chore: remove wrapper for unmaintained nanosim-h
in favor of new nanosim
wrapper (#3165)
#3143
chore: remove wrapper for unmaintained nanosim-h
in favor of new nanosim
wrapper (#3165)
#3143
Conversation
WalkthroughThe changes involve enhancements to the Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- bio/nanosim-h/meta.yaml (1 hunks)
- bio/nanosim-h/test/Snakefile (1 hunks)
- bio/nanosim-h/wrapper.py (2 hunks)
Additional context used
Path-based instructions (1)
bio/nanosim-h/wrapper.py (2)
Pattern
**/*.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 theself
argument of methods.
Do not suggest type annotation of thecls
argument of classmethods.
Do not suggest return type annotation if a function or method does not contain areturn
statement.
Pattern
**/wrapper.py
: Do not complain about use of undefined variable calledsnakemake
.
Ruff
bio/nanosim-h/wrapper.py
32-32: Undefined name
snakemake
(F821)
32-32: Undefined name
snakemake
(F821)
33-33: Undefined name
snakemake
(F821)
34-34: Undefined name
snakemake
(F821)
35-35: Undefined name
snakemake
(F821)
36-36: Undefined name
snakemake
(F821)
37-37: Undefined name
snakemake
(F821)
38-38: Undefined name
snakemake
(F821)
39-39: Undefined name
snakemake
(F821)
Additional comments not posted (11)
bio/nanosim-h/test/Snakefile (4)
3-3
: LGTM!The explicit definition of the
fasta
parameter improves clarity and usability.
6-8
: LGTM!The reformatting of the
output
section improves consistency.
10-14
: LGTM!The expanded
params
section enhances functionality and flexibility by allowing users to specify a broader range of parameters.
16-16
: LGTM!The update to the
log
section improves consistency with the new formatting style.bio/nanosim-h/meta.yaml (3)
2-5
: LGTM!The reformatting of the
description
to block style improves readability.
8-8
: LGTM!The addition of the
url
field enhances accessibility by providing a reference point for users.
10-24
: LGTM!The expanded
params
section with detailed descriptions improves the documentation quality, making it easier for users to understand how to configure the simulator effectively.bio/nanosim-h/wrapper.py (4)
32-39
: LGTM!The additional parameters enhance the function's capability to manage input data more effectively.
Tools
Ruff
32-32: Undefined name
snakemake
(F821)
32-32: Undefined name
snakemake
(F821)
33-33: Undefined name
snakemake
(F821)
34-34: Undefined name
snakemake
(F821)
35-35: Undefined name
snakemake
(F821)
36-36: Undefined name
snakemake
(F821)
37-37: Undefined name
snakemake
(F821)
38-38: Undefined name
snakemake
(F821)
39-39: Undefined name
snakemake
(F821)
32-39
: LGTM!The updates to the
prefix
parameter and the introduction ofinput_fasta
andinput_profile_dir
improve the flexibility and usability of the function.Tools
Ruff
32-32: Undefined name
snakemake
(F821)
32-32: Undefined name
snakemake
(F821)
33-33: Undefined name
snakemake
(F821)
34-34: Undefined name
snakemake
(F821)
35-35: Undefined name
snakemake
(F821)
36-36: Undefined name
snakemake
(F821)
37-37: Undefined name
snakemake
(F821)
38-38: Undefined name
snakemake
(F821)
39-39: Undefined name
snakemake
(F821)
60-60
: LGTM!The update to the command string ensures that the new input parameters are passed correctly in the command execution.
32-39
: Skip the static analysis hints.The use of
snakemake
is intentional and should not be flagged as an issue.Tools
Ruff
32-32: Undefined name
snakemake
(F821)
32-32: Undefined name
snakemake
(F821)
33-33: Undefined name
snakemake
(F821)
34-34: Undefined name
snakemake
(F821)
35-35: Undefined name
snakemake
(F821)
36-36: Undefined name
snakemake
(F821)
37-37: Undefined name
snakemake
(F821)
38-38: Undefined name
snakemake
(F821)
39-39: Undefined name
snakemake
(F821)
Just wondering what the advantage would be of having all parameters separately (instead of a single |
Yeah, I also saw that. Also, it might make sense to run stuff in a temporary directory and then copy to truly arbitrary file path and names. But I think I think it is not worth investing too much in this wrapper, as So for my own usage, I am now working on a wrapper for the original tool, |
Just fixed the formatting, so I hope all tests will run through, now. Feel free to review and merge, if you are satisfied, @fgvieira. Also, the newer |
Hmm, I can't seem to get Should we maybe remove this old wrapper altogether? It will still exist in the older |
Yep, if we cannot get it to work and @johanneskoester has nothing against it, I agree that it is better to just remove it. |
nanosim-h
input file handlingnanosim-h
in favor of new nanosim
wrapper (#3165)
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.
QC
snakemake-wrappers
.While the contributions guidelines are more extensive, please particularly ensure that:
test.py
was updated to call any added or updated example rules in aSnakefile
input:
andoutput:
file paths in the rules can be chosen arbitrarilyinput:
oroutput:
)tempfile.gettempdir()
points tometa.yaml
contains a link to the documentation of the respective tool or command underurl:
Summary by CodeRabbit
New Features
max_read_len
parameter to thenanosimh
rule, allowing for longer simulated reads.Improvements
Bug Fixes