-
Notifications
You must be signed in to change notification settings - Fork 187
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
feat: Add wrappers for ROOT rootcp CLI tool #3251
feat: Add wrappers for ROOT rootcp CLI tool #3251
Conversation
Warning Rate limit exceeded@GoodingJamie has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 19 minutes and 26 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe changes introduce several new files related to the Changes
Possibly related PRs
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 (
|
f07b6ca
to
a1352b6
Compare
Co-authored-by: Filipe G. Vieira <1151762+fgvieira@users.noreply.github.com>
a20816a
to
8144a54
Compare
Thanks a lot for your contributions! |
Thanks a bunch @fgvieira, all of the nice ways to tidy the wrapper up suggested above are in now 😁 |
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
Outside diff range and nitpick comments (2)
phys/root/rootcp/meta.yaml (1)
14-14
: Add a newline at the end of the file.To adhere to best practices and maintain consistency, please add a newline character at the end of the file.
Tools
yamllint
[error] 14-14: no new line character at the end of file
(new-line-at-end-of-file)
test.py (1)
Line range hint
12-24
: The current discount policy may have unintended consequences.Applying a flat $20 fee after the loyalty discount has the potential to negate the discount's benefit, especially for customers in the lower loyalty tiers making small purchases.
For example, a customer with 3 years of loyalty making a $100 purchase would see their bill discounted to $90 (10% off), but the $20 fee would bring it to $110, which is higher than the original undiscounted amount.
This may disincentivize customers from maintaining loyalty and cause frustration when the "discount" results in a higher total cost.
Consider adjusting the discount percentages and/or the flat fee logic to ensure the loyalty program appropriately incentivizes and rewards customers as intended. Some ideas:
- Increase the discount percentages to offset the $20 fee impact
- Make the fee a percentage of the discounted subtotal instead of a flat amount
- Waive the fee for lower subtotals or loyalty tiers
- Only apply the fee to the original total before the discount is applied
The exact adjustments will depend on your business goals, but the key is to avoid scenarios where the loyalty discount unintentionally penalizes customers.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- phys/root/rootcp/environment.linux-64.pin.txt (1 hunks)
- phys/root/rootcp/environment.yaml (1 hunks)
- phys/root/rootcp/meta.yaml (1 hunks)
- phys/root/rootcp/test/Snakefile (1 hunks)
- phys/root/rootcp/wrapper.py (1 hunks)
- test.py (1 hunks)
Files skipped from review due to trivial changes (1)
- phys/root/rootcp/environment.linux-64.pin.txt
Additional context used
Path-based instructions (2)
phys/root/rootcp/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
.test.py (1)
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.
Ruff
phys/root/rootcp/wrapper.py
9-9: Undefined name
snakemake
(F821)
10-10: Undefined name
snakemake
(F821)
12-12: Undefined name
snakemake
(F821)
16-16: Undefined name
snakemake
(F821)
yamllint
phys/root/rootcp/meta.yaml
[error] 14-14: no new line character at the end of file
(new-line-at-end-of-file)
Additional comments not posted (8)
phys/root/rootcp/environment.yaml (2)
1-2
: LGTM!Using the
conda-forge
channel is a good choice as it provides a wide range of community-maintained packages.
3-4
: LGTM!The ROOT version 6.30.4 is consistent with the past review comment. Good job addressing the feedback!
phys/root/rootcp/test/Snakefile (1)
1-13
: LGTM!The
rootcp
rule is well-structured and follows the Snakemake syntax. It enhances the functionality of the workflow by enabling automated copying of ROOT files with specified configurations.Some key observations:
- The rule specifies input, log, params, threads, output, and wrapper, providing a clear and maintainable structure.
- The wrapper abstraction allows for a clean implementation by encapsulating the copying logic.
- The logging configuration ensures that the process can be monitored and debugged if needed.
- The parameters allow for customization of the copying process, such as specifying the input object name and extra options.
Overall, the rule is well-designed and adds value to the workflow.
phys/root/rootcp/wrapper.py (1)
1-22
: LGTM!The script follows the expected structure and logic for a Snakemake wrapper. It correctly handles optional parameters, executes the
rootcp
command, and logs the output. The code changes are well-implemented and provide flexibility to the user.Tools
Ruff
9-9: Undefined name
snakemake
(F821)
10-10: Undefined name
snakemake
(F821)
12-12: Undefined name
snakemake
(F821)
16-16: Undefined name
snakemake
(F821)
phys/root/rootcp/meta.yaml (3)
1-5
: LGTM!The metadata section is well-defined, providing essential information about the
rootcp
tool, including its name, description, documentation URL, and author. This enhances the discoverability and usability of the tool.
6-10
: LGTM!The input and output specifications are well-defined, clearly indicating the expected input (ROOT file/object(s) target and destination) and output (single ROOT file containing copied target object(s)). This clarity helps users understand the tool's expected input and output.
11-14
: LGTM!The parameters section is well-defined, providing optional and extra parameters for the
rootcp
tool. Theextra
parameter allows users to pass additional parameters torootcp
as documented by ROOT, while theinput_object_name
andoutput_object_name
parameters offer granular control over the copying process. This enhances the tool's flexibility and usability.Tools
yamllint
[error] 14-14: no new line character at the end of file
(new-line-at-end-of-file)
test.py (1)
6855-6860
: Looks good!The new
test_root_rootcp
function properly tests the "phys/root/rootcp" wrapper using the standard pattern. Nice work.
🤖 I have created a release \*beep\* \*boop\* --- ## [4.5.0](https://www.github.com/snakemake/snakemake-wrappers/compare/v4.4.0...v4.5.0) (2024-09-20) ### Features * Add wrappers for ROOT rootcp CLI tool ([#3251](https://www.github.com/snakemake/snakemake-wrappers/issues/3251)) ([0be5d56](https://www.github.com/snakemake/snakemake-wrappers/commit/0be5d566f4767b7cd2ea9ba78b0d83a6f79a4803)) * Bump meryl version ([#3266](https://www.github.com/snakemake/snakemake-wrappers/issues/3266)) ([448a1cb](https://www.github.com/snakemake/snakemake-wrappers/commit/448a1cb793d04f7bd280c36bc4dd37d2d06aa104)) * Enhance phys/root/filter functionality ([#3250](https://www.github.com/snakemake/snakemake-wrappers/issues/3250)) ([4797d76](https://www.github.com/snakemake/snakemake-wrappers/commit/4797d76630b0cc6ea05778a49727f7917b7874dc)) * Parse threads ([#3249](https://www.github.com/snakemake/snakemake-wrappers/issues/3249)) ([9e63554](https://www.github.com/snakemake/snakemake-wrappers/commit/9e63554b0cf19b2a22513566a576105c39f47e3b)) ### Bug Fixes * name of bamqc ([#1464](https://www.github.com/snakemake/snakemake-wrappers/issues/1464)) ([ee04ec2](https://www.github.com/snakemake/snakemake-wrappers/commit/ee04ec22b24c8d380ef98f5cee677f4ff4730ad3)) ### Performance Improvements * autobump bio/cnv_facets ([#3253](https://www.github.com/snakemake/snakemake-wrappers/issues/3253)) ([c5c8ddd](https://www.github.com/snakemake/snakemake-wrappers/commit/c5c8ddded41ba96fd8bbc69790e1e17998551734)) * autobump bio/emu/abundance ([#3256](https://www.github.com/snakemake/snakemake-wrappers/issues/3256)) ([6e42aef](https://www.github.com/snakemake/snakemake-wrappers/commit/6e42aef12570e7708dedd4ed24a7406a69356d81)) * autobump bio/emu/collapse-taxonomy ([#3255](https://www.github.com/snakemake/snakemake-wrappers/issues/3255)) ([969067e](https://www.github.com/snakemake/snakemake-wrappers/commit/969067e8a94210d99bb67dfb3525c076f7731d02)) * autobump bio/emu/combine-outputs ([#3254](https://www.github.com/snakemake/snakemake-wrappers/issues/3254)) ([de2a1be](https://www.github.com/snakemake/snakemake-wrappers/commit/de2a1bef7e9d330c4d6484bf0f1f250d7ad6c0c9)) * autobump bio/freebayes ([#3257](https://www.github.com/snakemake/snakemake-wrappers/issues/3257)) ([80630dd](https://www.github.com/snakemake/snakemake-wrappers/commit/80630dd19aa113ea94dd55f89f596b83e81ebc34)) * autobump bio/galah ([#3258](https://www.github.com/snakemake/snakemake-wrappers/issues/3258)) ([285d57a](https://www.github.com/snakemake/snakemake-wrappers/commit/285d57a8dd082fb515250fdc370cca11142fff44)) * autobump bio/gdc-api/bam-slicing ([#3259](https://www.github.com/snakemake/snakemake-wrappers/issues/3259)) ([27b6958](https://www.github.com/snakemake/snakemake-wrappers/commit/27b695863bc123ba93fff53a130a0d7a06b4b2c1)) * autobump bio/igv-reports ([#3260](https://www.github.com/snakemake/snakemake-wrappers/issues/3260)) ([a7d57ba](https://www.github.com/snakemake/snakemake-wrappers/commit/a7d57ba191bb59060dc82b9009a11c78dbaba86e)) * autobump bio/lofreq/call ([#3262](https://www.github.com/snakemake/snakemake-wrappers/issues/3262)) ([13626f0](https://www.github.com/snakemake/snakemake-wrappers/commit/13626f0b9d3d25bafd04a3253f37b6bfd91414bc)) * autobump bio/lofreq/indelqual ([#3261](https://www.github.com/snakemake/snakemake-wrappers/issues/3261)) ([76c854e](https://www.github.com/snakemake/snakemake-wrappers/commit/76c854e127cd792b5f74f8dc357f09fddb07998c)) * autobump bio/multiqc ([#3263](https://www.github.com/snakemake/snakemake-wrappers/issues/3263)) ([d4d1475](https://www.github.com/snakemake/snakemake-wrappers/commit/d4d14750f10aa5f10fd5b20f560e13985a0f758f)) * autobump bio/tabix/index ([#3264](https://www.github.com/snakemake/snakemake-wrappers/issues/3264)) ([e39e97e](https://www.github.com/snakemake/snakemake-wrappers/commit/e39e97e96fa26ab40e34a207ed62410453d28bae)) * autobump bio/vep/annotate ([#3265](https://www.github.com/snakemake/snakemake-wrappers/issues/3265)) ([7f0b02a](https://www.github.com/snakemake/snakemake-wrappers/commit/7f0b02ac64b40a5aca8bd08c90f8b7df80ea4bed)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Adds Snakemake wrappers for the ROOT command line tool
rootcp
.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
rootcp
tool for copying ROOT files via command line, enhancing usability with detailed input and output specifications.Bug Fixes
rootcp
process, ensuring reliability.