add r-chromvarmotifs
#51395
add r-chromvarmotifs
#51395
Conversation
📝 WalkthroughWalkthroughThis pull request introduces several new components for the Possibly related PRs
Suggested labels
Suggested reviewers
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
recipes/r-chromvarmotifs/build.sh (2)
3-3: Consider addressing the underlying issue instead of disabling ShellCheckThe ShellCheck directive
# shellcheck disable=SC2086is used to suppress warnings about unquoted variables. While this might be necessary due to how${R_ARGS}is used, it's generally better to address the underlying issue rather than disable the check.Consider modifying the next line to properly quote the variables, which would allow you to remove this ShellCheck disable directive. For example:
"${R}" CMD INSTALL --build . "${R_ARGS}"This change would maintain the intended functionality while adhering to shell scripting best practices.
4-4: LGTM with a minor suggestion: Quote variables for safetyThe R installation command is correct and uses the standard approach for installing R packages in build scripts. The use of
${R}and${R_ARGS}variables allows for flexibility across different environments, which is good.However, to address potential word splitting or globbing issues, consider modifying the line as follows:
"${R}" CMD INSTALL --build . "${R_ARGS}"This change ensures that the variables are properly quoted, preventing any unexpected behavior if
${R_ARGS}contains spaces or special characters.recipes/r-chromvarmotifs/meta.yaml (2)
9-11: LGTM: Source URL and checksum are correctly defined.The use of a specific commit is appropriate when there's no proper release from the upstream repository. The SHA256 checksum ensures the integrity of the downloaded source.
Consider adding a comment explaining why a specific commit is used instead of a release version. This will help future maintainers understand the reasoning behind this choice.
43-45: Consider adding more maintainers.Having a designated maintainer is crucial. However, to ensure continued maintenance and support, it might be beneficial to have multiple maintainers.
Consider reaching out to other contributors or maintainers of related packages to see if they would be interested in co-maintaining this package. This can help distribute the workload and ensure long-term support for the package.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- recipes/r-chromvarmotifs/build.sh (1 hunks)
- recipes/r-chromvarmotifs/conda_build_config.yaml (1 hunks)
- recipes/r-chromvarmotifs/meta.yaml (1 hunks)
🧰 Additional context used
🪛 yamllint
recipes/r-chromvarmotifs/meta.yaml
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
🔇 Additional comments (8)
recipes/r-chromvarmotifs/conda_build_config.yaml (1)
1-4: LGTM! Good job on including multiple R versions.The configuration looks good and includes R versions 4.1, 4.2, and 4.3. This is particularly important as it includes R 4.1, which is crucial for the stability of the
ArchRpackage as mentioned in the PR objectives. Including newer versions (4.2 and 4.3) also future-proofs the package.To ensure these R versions align with the package's compatibility requirements, please run the following script:
This script will help verify that the R versions specified in
conda_build_config.yamlare consistent with any version constraints or specific commands inmeta.yamlandbuild.sh.recipes/r-chromvarmotifs/build.sh (3)
1-1: LGTM: Correct shebang for a bash scriptThe shebang
#!/bin/bashis correct and follows the standard practice for bash scripts.
2-2: LGTM: Disabling Autobrew is appropriate for Conda recipesSetting
DISABLE_AUTOBREW=1is correct. This prevents the automatic use of Homebrew for dependencies, which is appropriate in a Conda environment where we want to use Conda-managed dependencies.
1-4: Overall assessment: Good script with minor improvements suggestedThis build script for the r-chromvarmotifs package is well-structured and follows common practices for Conda recipe build scripts. It correctly sets up the environment and uses the standard R installation command.
Key points:
- The shebang and DISABLE_AUTOBREW setting are correct.
- The R installation command is appropriate for building the package.
Suggestions for improvement:
- Consider quoting variables in the R installation command to prevent potential issues with word splitting or globbing.
- If possible, address the underlying issue that necessitated the ShellCheck disable directive.
These minor adjustments will enhance the script's robustness and adherence to shell scripting best practices.
recipes/r-chromvarmotifs/meta.yaml (4)
1-7: LGTM: Package name and version are correctly defined.The package name follows the Bioconda naming convention for R packages, and the version is set using Jinja2 templating, which allows for easier updates.
🧰 Tools
🪛 yamllint
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
13-20: LGTM: Build section is correctly configured.The build section is well-configured:
- The
noarch: genericsetting is appropriate for R packages.- The build number is correctly set to 0 for a new package.
- The RPaths are correctly set for R libraries.
- The use of
run_exportshelps manage dependency versions effectively.
34-41: LGTM: About section is comprehensive and accurate.The about section provides all necessary information:
- The home URL is correctly set to the GitHub repository.
- The license is specified as MIT, with the correct license family and files.
- A concise summary of the package's purpose is provided.
1-1: Note on yamllint warning: False positiveThe yamllint tool reports a syntax error for the '%' character at the beginning of the file. This is a false positive as the '%' character is part of the Jinja2 templating syntax, which is valid and commonly used in Conda recipe files.
No action is required to address this warning.
🧰 Tools
🪛 yamllint
[error] 1-1: syntax error: found character '%' that cannot start any token
(syntax)
| requirements: | ||
| host: | ||
| - r-base | ||
| - bioconductor-tfbstools | ||
| run: | ||
| - r-base | ||
| - bioconductor-tfbstools |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider pinning the r-base version.
The requirements section includes the necessary dependencies. However, to ensure compatibility and reproducibility, it's recommended to pin the r-base version.
Consider updating the r-base requirement to specify a version range. For example:
requirements:
host:
- r-base {{ r_base }}
- bioconductor-tfbstools
run:
- r-base {{ r_base }}
- bioconductor-tfbstoolsThis assumes that {{ r_base }} is defined in your conda_build_config.yaml file. If not, you may need to specify the version explicitly, e.g., r-base >=4.1,<4.4.
| test: | ||
| commands: | ||
| - $R -e "library(chromVARmotifs)" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding more comprehensive tests.
The current test command checks if the package can be loaded, which is a good basic test. However, to ensure the package functions correctly, consider adding more comprehensive tests.
You could add additional test commands to verify specific functionalities of the package. For example:
test:
commands:
- $R -e "library(chromVARmotifs)"
- $R -e "stopifnot(length(chromVARmotifs::JASPAR2022_human) > 0)"
- $R -e "stopifnot(is(chromVARmotifs::JASPAR2022_human, 'PWMatrixList'))"These additional tests check if the package contains the expected data and if it's of the correct type.
Adds the GitHub R package
chromVARmotifsasr-chromvarmotifs. Unfortunately, upstream lacks a proper release (Issue posted), so we specify the commit in the meantime.Notably, this is required by the
devbranch ofArchR. Since ArchR stability is currently only guaranteed for R 4.1, we additionally make an R 4.1 variant here. Some related discussion in #51295.Please read the guidelines for Bioconda recipes before opening a pull request (PR).
General instructions
@BiocondaBot please add labelcommand.@bioconda/corein a comment.Instructions for avoiding API, ABI, and CLI breakage issues
Conda is able to record and lock (a.k.a. pin) dependency versions used at build time of other recipes.
This way, one can avoid that expectations of a downstream recipe with regards to API, ABI, or CLI are violated by later changes in the recipe.
If not already present in the meta.yaml, make sure to specify
run_exports(see here for the rationale and comprehensive explanation).Add a
run_exportssection like this:with
...being one of:{{ pin_subpackage("myrecipe", max_pin="x") }}{{ pin_subpackage("myrecipe", max_pin="x.x") }}{{ pin_subpackage("myrecipe", max_pin="x.x") }}(in such a case, please add a note that shortly mentions your evidence for that){{ pin_subpackage("myrecipe", max_pin="x.x.x") }}(in such a case, please add a note that shortly mentions your evidence for that){{ pin_subpackage("myrecipe", max_pin=None) }}while replacing
"myrecipe"with eithernameif aname|lowervariable is defined in your recipe or with the lowercase name of the package in quotes.Bot commands for PR management
Please use the following BiocondaBot commands:
Everyone has access to the following BiocondaBot commands, which can be given in a comment:
@BiocondaBot please update@BiocondaBot please add labelplease review & mergelabel.@BiocondaBot please fetch artifactsYou can use this to test packages locally.
Note that the
@BiocondaBot please mergecommand is now depreciated. Please just squash and merge instead.Also, the bot watches for comments from non-members that include
@bioconda/<team>and will automatically re-post them to notify the addressed<team>.