Open
Conversation
Reviewer's GuideAdds configurable sorting for ASCII-style alignment pileups (by count or by position, with optional reversal), wires it through the Snakemake rules and config, documents the behavior (including insertion handling), and bumps the script version. File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- Consider validating
--sort-byagainst the supported values ("position" and "counts") early inascii_alignment_pileup.Rand failing fast on invalid input instead of silently falling back to theelsebranch. - The new
--reverse-sortCLI flag is defined inascii_alignment_pileup.Rbut never passed from the Snakemake rules, so the workflow currently cannot change the sort direction; either wire it through the rules or document it as a script-only option. - In
docs/api/ascii_alignment_pileup.R.mdthe option list entry for the reverse-sort flag is missing the leading--(it appears asreverse-sortinstead of--reverse-sort), which may confuse users when calling the script.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider validating `--sort-by` against the supported values ("position" and "counts") early in `ascii_alignment_pileup.R` and failing fast on invalid input instead of silently falling back to the `else` branch.
- The new `--reverse-sort` CLI flag is defined in `ascii_alignment_pileup.R` but never passed from the Snakemake rules, so the workflow currently cannot change the sort direction; either wire it through the rules or document it as a script-only option.
- In `docs/api/ascii_alignment_pileup.R.md` the option list entry for the reverse-sort flag is missing the leading `--` (it appears as `reverse-sort` instead of `--reverse-sort`), which may confuse users when calling the script.
## Individual Comments
### Comment 1
<location path="workflow/scripts/ascii_alignment_pileup.R" line_range="141" />
<code_context>
+ action="store",
+ type="character",
+ default="counts",
+ help="Specify the sort type (either \"position\" or \"counts\").
+ [default \"%default\"]",
+ metavar="string"
</code_context>
<issue_to_address>
**issue (bug_risk):** The help string is split across lines in a way that will break R string syntax.
This help string will cause a parse error because double-quoted strings in R cannot span multiple physical lines. Please either keep the string on one line, insert an explicit `\n` before `[default "%default"]`, or build it with `paste()` or a similar approach.
</issue_to_address>
### Comment 2
<location path="workflow/scripts/ascii_alignment_pileup.R" line_range="140" />
<code_context>
+ "--sort-by",
+ action="store",
+ type="character",
+ default="counts",
+ help="Specify the sort type (either \"position\" or \"counts\").
+ [default \"%default\"]",
</code_context>
<issue_to_address>
**question:** CLI default for --sort-by differs from the workflow config default.
The CLI default for this flag is "counts", whereas `config_template.yaml` uses `sort_by: 'position'`. Via the Snakemake workflow, the config overrides this, but when running the script directly, users see different default behavior. If this difference isn’t intentional, please align the defaults; if it is, consider documenting the distinction explicitly rather than relying only on `%default`.
</issue_to_address>
### Comment 3
<location path="docs/api/ascii_alignment_pileup.R.md" line_range="148-153" />
<code_context>
not provided the input BAM file(s) name will be used instead.
+- <b>`--sort-by=STRING`</b>: Specify the sort type (either "position" or
+ "counts"). (default = "counts")
+- <b>`reverse-sort`</b>: Reverse the sort order; from "from-left-to-right" to
+ "from-right-to-left" for sort type "position", and from "descending" to
+ "ascending" for sort type "counts".
</code_context>
<issue_to_address>
**issue (typo):** Option name is missing leading dashes compared to earlier usage of `--reverse-sort`.
In the options list this flag is shown as `reverse-sort` (no leading `--`), which is inconsistent with the earlier `--reverse-sort` usage and the actual CLI. Please update this entry to `<b>
--reverse-sort`</b>` to match.
```suggestion
- <b>`--sort-by=STRING`</b>: Specify the sort type (either "position" or
"counts"). (default = "counts")
- <b>`--reverse-sort`</b>: Reverse the sort order; from "from-left-to-right" to
"from-right-to-left" for sort type "position", and from "descending" to
"ascending" for sort type "counts".
- <b>`-h`</b> | <b>`--help`</b>: Show this information and die.
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
uniqueg
requested changes
Mar 16, 2026
Member
uniqueg
left a comment
There was a problem hiding this comment.
Looks good. One minor thing: best to define sort_by as an enumerator, which can only take on the values position (default) or counts, then validate the user choice via an optparse custom validator.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes #145.
In addition, the treatment of insertions within an alignment's sequence has been documented where required.
Conventional Commits
Conventional Commits specification
Checklist
works
reduced the code coverage relative to the previous state
affected by the proposed changes
Summary by Sourcery
Add configurable sorting for ASCII-style alignment pileups and document alignment insertion handling.
New Features:
Enhancements:
Documentation:
Tests: