Configuration used: CodeRabbit UI
Review profile: CHILL

#!/bin/bash
# Description: Check latest version and potential issues for picard-slim

# Get latest version from conda-forge
curl -s https://conda.anaconda.org/bioconda/noarch/repodata.json | jq -r '.packages | to_entries[] | select(.value.name=="picard-slim") | .value.version' | sort -V | tail -n 1

# Check for any reported issues in the past 6 months
gh api graphql -f query='
{
  search(query: "repo:broadinstitute/picard is:issue created:>2024-06-01", type: ISSUE, first: 5) {
    nodes {
      ... on Issue {
        title
        url
        labels(first: 5) {
          nodes {
            name
          }
        }
      }
    }
  }
}'

#!/bin/bash
# Description: Check for known compatibility issues between the selected versions

# Check release notes and issues for compatibility information
gh api graphql -f query='
{
  repository(owner: "broadinstitute", name: "picard") {
    release(tagName: "3.3.0") {
      description
    }
  }
}
'

# Check if there are any reported issues between these specific versions
gh api graphql -f query='
{
  search(query: "repo:samtools/samtools repo:broadinstitute/picard is:issue samtools 1.21 picard 3.3.0", type: ISSUE) {
    nodes {
      ... on Issue {
        title
        url
        state
      }
    }
  }
}
'

#!/bin/bash
# Let's check for any compatibility requirements or issues in the wrapper script and workflow

# Check wrapper script for version requirements
fd "bowtie2.*\.py" --exec cat {}

# Check workflow for version requirements
fd "bowtie2.*\.smk|bowtie2.*\.snakefile" -I --exec cat {}

# Check if there are any version constraints in environment files
fd "environment.*\.ya?ml" --exec grep -A5 -B5 "bowtie2\|samtools\|picard" {}

# Look for any version-specific code or comments
rg -A3 -B3 "version|Version" --glob "bowtie2*"

bio/bowtie2/align/environment.linux-64.pin.txt (1)

16-55: Consider optimizing system library dependencies.

The environment includes a comprehensive set of system libraries. While these are likely pulled in as dependencies, consider:

1. Evaluating if all these libraries are necessary for the core functionality
2. Testing if any can be marked as optional dependencies

bio/bowtie2/align/wrapper.py (2)

42-45: Fix typo in error message

There's a typo in the error message: "declarad" should be "declared".

-        f"Missing required indices: {missing_idx} declarad as input.idx.\n"
+        f"Missing required indices: {missing_idx} declared as input.idx.\n"

---

171-179: Consider resource management optimization

The wrapper combines alignment and sorting in a single step using pipes. While this is memory-efficient, it could benefit from some resource management considerations:

1. For large datasets, consider adding a parameter to control the memory usage of sorting tools (e.g., --memory for samtools, MAX_RECORDS_IN_RAM for Picard)
2. Consider adding progress reporting for long-running operations

Would you like help implementing these resource management optimizations?

bio/bowtie2/align/meta.yaml (1)

Line range hint 29-31: Enhance documentation for new sorting parameters

The notes section should document the new sorting functionality and parameters.

Add documentation for the new parameters:

  * The `extra` param allows for additional arguments for bowtie2.
+ * The `sort_program` param enables output sorting using either 'samtools' or 'picard'. Default is 'none'.
+ * The `sort_extra` param allows passing additional arguments to the selected sorting program.
+ * The `sort_order` param specifies the sorting order: 'coordinate' (default) or 'queryname'.
  * This wrapper uses an inner pipe. Make sure to use at least two threads in your Snakefile.

bio/bowtie2/align/test/Snakefile (2)

140-140: Consider dynamic thread allocation for sorting

The current fixed thread count might not be optimal for all sorting operations. Consider adjusting threads based on the sorting program.

-               threads: 8  # Use at least two threads
+               threads: lambda wildcards, input: 16 if wildcards.program != "none" else 8  # Increase threads for sorting operations

---

138-139: Enhance log file structure

Consider organizing log files in subdirectories based on sorting configuration for better maintainability.

-               log:
-                   f"logs/bowtie2/{{sample}}.{program}_{order}_{extension}.log",
+               log:
+                   f"logs/bowtie2/{program}/{order}/{{sample}}.{extension}.log",

Configuration used: CodeRabbit UI
Review profile: CHILL

#!/bin/bash
# Description: Verify compatibility between tool versions

# Check if there are any known compatibility issues between these versions
gh api graphql -f query='
{
  search(query: "repo:broadinstitute/picard is:issue \"bowtie2 2.5.4\" OR \"samtools 1.21\"", type: ISSUE) {
    nodes {
      ... on Issue {
        title
        url
        state
      }
    }
  }
}'

# Check if there are any reported issues with these specific versions
rg -l "compatibility|issue|error" | xargs rg -A 5 "bowtie2.*2\.5\.4|samtools.*1\.21|picard.*3\.3\.0"

#!/bin/bash
# Let's try a different approach to verify compatibility

# 1. Check for any configuration files or workflow files that might show these tools working together
fd -e yaml -e yml -e config -e json | xargs rg -l "bowtie2|samtools|picard"

# 2. Look for any documented version constraints or requirements
fd "README|CHANGELOG|requirements" | xargs rg -i "version|compatibility|require"

# 3. Check for any test files that might use these tools together
fd -e test -e spec | xargs rg -l "bowtie2|samtools|picard"

# 4. Look for workflow definitions that might show tool interactions
fd "workflow|pipeline|snakefile|nextflow" -i | xargs rg -l "bowtie2|samtools|picard"

# 5. Check if there are any documented issues in issue templates or docs
fd "ISSUE_TEMPLATE|docs" -i | xargs rg -i "compatibility|version"

-    raise ValueError(f"Unexpected value for params.sort ({sort})")
+    raise ValueError(f"Unexpected value for params.sort_program ({sort_program})")

     bowtie2_threads = bowtie2_threads - 1
     if bowtie2_threads <= 0:
         raise ValueError(
-            "Not enough threads requested. This wrapper requires exactly one more."
+            "Not enough threads requested. When using Picard, at least 2 threads are required (1 for bowtie2, 1 for Picard)."
         )

#!/bin/bash
# Description: Check if all expected rule combinations are generated

# List all rules in the Snakefile
snakemake --list-rules 2>/dev/null | grep "bowtie2_align_" | sort

#!/bin/bash
# Let's examine the Snakefile content directly to verify the rule generation
cat bio/bowtie2/align/test/Snakefile

#!/bin/bash
# Search for rule definitions in the Snakefile
rg "rule bowtie2_align_" bio/bowtie2/align/test/Snakefile -A 5

#!/bin/bash
# Check the full context around the loops
rg -B 5 -A 10 'for order in \["coordinate", "queryname"\]' bio/bowtie2/align/test/Snakefile

bio/bowtie2/align/wrapper.py (1)

41-47: Fix typo in error message

There's a typo in the error message: "declarad" should be "declared".

-        f"Missing required indices: {missing_idx} declarad as input.idx.\n"
+        f"Missing required indices: {missing_idx} declared as input.idx.\n"

🧰 Tools

🪛 Ruff (0.8.0)

41-41: Undefined name snakemake

(F821)

Configuration used: CodeRabbit UI
Review profile: CHILL

-            "Not enough threads requested. This wrapper requires exactly one more."
+            "Not enough threads requested. Picard sorting requires at least 2 threads (1 for Bowtie2, 1 for Picard)."

-    raise ValueError(f"Unexpected value for params.sort ({sort_program})")
+    raise ValueError(f"Unexpected value for params.sort_program ({sort_program})")

bio/bowtie2/align/wrapper.py (4)

14-23: Add edge case handling to get_extension function

The function should handle edge cases such as empty filenames or files without extensions to prevent potential runtime errors.

Consider this improved implementation:

 def get_extension(filename: str) -> str:
     """
     Return file format since Bowtie2 reads files that
     could be gzip'ed (extension: .gz) or bzip2'ed (extension: .bz2).
     """
+    if not filename:
+        raise ValueError("Empty filename")
+    parts = filename.split(".")
+    if len(parts) < 2:
+        raise ValueError(f"No extension found in filename: {filename}")
     if filename.endswith((".gz", ".bz2")):
-        return filename.split(".")[-2].lower()
+        return parts[-2].lower() if len(parts) > 2 else ""
-    return filename.split(".")[-1].lower()
+    return parts[-1].lower()

---

45-47: Fix typo in error message

There's a typo in the error message: "declarad" should be "declared".

-        f"Missing required indices: {missing_idx} declarad as input.idx.\n"
+        f"Missing required indices: {missing_idx} declared as input.idx.\n"

---

155-183: Consider using constants for repeated string literals

The code uses string literals like "none", "samtools", "picard" multiple times. Consider defining these as constants at the module level to improve maintainability.

+# Constants for sort programs
+SORT_PROGRAM_NONE = "none"
+SORT_PROGRAM_SAMTOOLS = "samtools"
+SORT_PROGRAM_PICARD = "picard"
+
 match sort_program:
-    case "none":
+    case SORT_PROGRAM_NONE:
         # ...
-    case "samtools":
+    case SORT_PROGRAM_SAMTOOLS:
         # ...
-    case "picard":
+    case SORT_PROGRAM_PICARD:
         # ...

---

111-116: Consider extracting CRAM validation to a separate function

The CRAM validation logic could be moved to a separate function to improve code organization and reusability.

+def validate_cram_requirements(ref: str | None, ref_fai: str | None) -> None:
+    """Validate that reference files are provided for CRAM output."""
+    if ref is None or ref_fai is None:
+        raise ValueError(
+            "Reference file and index are required for CRAM output. "
+            "Please specify them as input.ref and input.ref_fai."
+        )
+
 if bam_extension == "cram":
-    if REF is None or REF_FAI is None:
-        raise ValueError(
-            "Reference file and index are required for CRAM output."
-            "Please specify them as input.ref and input.ref_fai."
-        )
+    validate_cram_requirements(REF, REF_FAI)

🧰 Tools

🪛 Ruff (0.8.0)

111-112: Use a single if statement instead of nested if statements

Combine if statements using and

(SIM102)

Configuration used: CodeRabbit UI
Review profile: CHILL

bio/bowtie2/align/test/Snakefile (2)

107-144: Consider validating program and order combinations

The current implementation allows all combinations of program and order, but some might not make sense (e.g., program="none" with order="coordinate"/"queryname"). Consider:

1. Adding documentation about valid combinations
2. Implementing validation logic to skip invalid combinations

Example improvement:

# Define valid combinations
VALID_COMBINATIONS = {
    "none": [""],  # no sorting
    "samtools": ["coordinate", "queryname"],
    "picard": ["coordinate", "queryname"]
}

for program, valid_orders in VALID_COMBINATIONS.items():
    for order in valid_orders:
        for extension in ["sam", "bam", "cram"]:
            # Rule definition here

---

129-134: Clean up commented output parameters

The commented output parameters appear to be unused. If they're not needed, consider removing them for better code clarity.

                output:
                    f"mapped_idx/{{sample}}.{program}_{order}.{extension}",
-                    # idx="",
-                    # metrics="",
-                    # unaligned="",
-                    # unpaired="",
-                    # unconcordant="",
-                    # concordant="",

Configuration used: CodeRabbit UI
Review profile: CHILL

bio/bowtie2/align/wrapper.py (3)

15-24: Consider handling additional edge cases in get_extension

While the function works correctly for standard cases, consider handling these edge cases:

1. Files without extensions
2. Files with multiple dots in the name
3. Case-insensitive extension comparison

 def get_extension(filename: str) -> str:
     """
     Return file format since Bowtie2 reads files that
     could be gzip'ed (extension: .gz) or bzip2'ed (extension: .bz2).
     """
+    if not filename or "." not in filename:
+        raise ValueError("Invalid filename: must have an extension")
     if filename.endswith((".gz", ".bz2")):
-        return filename.split(".")[-2].lower()
+        parts = filename.lower().split(".")
+        return parts[-2] if len(parts) > 2 else ""
-    return filename.split(".")[-1].lower()
+    return filename.lower().split(".")[-1]

---

174-176: Enhance CRAM output validation

When using CRAM output with samtools, consider validating that the reference file exists and is readable.

         if bam_extension == "cram":
+            if not path.isfile(REF):
+                raise ValueError(f"Reference file not found or not readable: {REF}")
             samtools_opts += f" --reference {REF} "

---

205-210: Consider using a list for command construction

The command construction could be more maintainable by using a list of command parts.

-    shell(
-        "( bowtie2"
-        " --threads {bowtie2_threads}"
-        " {CMD_INPUT} "
-        " -x {index}"
-        " {extra}"
-        " " + PIPE_CMD + ") {LOG}"
-    )
+    cmd_parts = [
+        "(",
+        "bowtie2",
+        f"--threads {bowtie2_threads}",
+        CMD_INPUT,
+        f"-x {index}",
+        extra,
+        PIPE_CMD,
+        f") {LOG}"
+    ]
+    shell(" ".join(cmd_parts))

Configuration used: CodeRabbit UI
Review profile: CHILL

-        f"Missing required indices: {missing_idx} declarad as input.idx.\n"
+        f"Missing required indices: {missing_idx} declared as input.idx.\n"

+if snakemake.threads < 2:
+    raise ValueError("This wrapper requires at least 2 threads")
 bowtie2_threads = snakemake.threads
 sort_threads = snakemake.threads - 1

bio/bowtie2/align/test/Snakefile (2)

136-136: Consider making threads configurable

The hardcoded thread count of 8 might not be optimal for all environments. Consider making this configurable through the config file or workflow resources.

-                threads: 8  # Use at least two threads
+                threads: config.get("bowtie2_threads", 8)  # Configurable with default

---

132-132: Document sort_extra parameter usage

The sort_extra parameter is included but empty. Please add a comment documenting its purpose and expected values for different sorting programs (samtools/picard).

-                    sort_extra="",
+                    sort_extra="",  # Additional arguments for sort program (e.g., "-m 4G" for samtools, "MAX_RECORDS_IN_RAM=1000000" for Picard)

Configuration used: CodeRabbit UI
Review profile: CHILL

bio/bowtie2/align/wrapper.py (1)

46-48: ⚠️ Potential issue

Improve thread allocation logic

The current thread allocation might lead to issues when snakemake.threads is 1.

Add validation before thread allocation:

 THREADS = snakemake.threads
+if THREADS < 2:
+    raise ValueError("This wrapper requires at least 2 threads")
 sort_threads = snakemake.threads - 1

🧰 Tools

🪛 Ruff (0.8.2)

46-46: Undefined name snakemake

(F821)

---

47-47: Undefined name snakemake

(F821)

bio/bowtie2/align/meta.yaml (1)

17-21: Consider documenting optional outputs in notes section

The commented optional output paths could be moved to the notes section to maintain cleaner YAML structure while preserving the documentation of available outputs.

output:
  - SAM/BAM/CRAM file. This must be the first output file in the list.
  - idx: Optional path to bam index.
-  # - metrics: Optional path to metrics file.
-  # - unaligned: Optional path to unaligned unpaired reads.
-  # - unpaired: Optional path to unpaired reads that aligned at least once.
-  # - unconcordant: Optional path to pairs that didn't align concordantly.
-  # - concordant: Optional path to pairs that aligned concordantly at least once.
notes: |
  * The `extra` param allows for additional arguments for bowtie2.
  * This wrapper uses an inner pipe. Make sure to use at least two threads in your Snakefile.
+ * Optional outputs:
+   - metrics: Path to metrics file
+   - unaligned: Path to unaligned unpaired reads
+   - unpaired: Path to unpaired reads that aligned at least once
+   - unconcordant: Path to pairs that didn't align concordantly
+   - concordant: Path to pairs that aligned concordantly at least once

bio/bowtie2/align/wrapper.py (5)

16-23: Add input validation to get_extension

While the function handles basic cases well, it could benefit from input validation to handle edge cases.

Consider this enhanced implementation:

 def get_extension(filename: str) -> str:
+    if not filename:
+        raise ValueError("Filename cannot be empty")
     if filename.endswith((".gz", ".bz2")):
         return filename.split(".")[-2].lower()
     return filename.split(".")[-1].lower()

---

65-66: Enhance error message for input validation

The error message could be more descriptive to help users understand the expected format.

Consider this improvement:

 if not isinstance(SAMPLE, str) and len(SAMPLE) not in [1, 2]:
-    raise ValueError("input must have 1 (single-end) or 2 (paired-end) elements")
+    raise ValueError(
+        f"Input must have 1 (single-end) or 2 (paired-end) elements, got {len(SAMPLE)} elements"
+    )

---

106-110: Improve error messages for sort parameters

The error messages for sort parameters could be more user-friendly.

Consider these improvements:

 if SORT_ORDER not in {"coordinate", "queryname"}:
     raise ValueError(
-        f"Unexpected value for sort_order ({SORT_ORDER})"
-        "Valid values are 'coordinate' or 'queryname'"
+        f"Invalid sort_order '{SORT_ORDER}'. Valid values are: 'coordinate' or 'queryname'"
     )

 if SORT_PROGRAM not in {"none", "samtools", "picard"}:
     raise ValueError(
-        f"Unexpected value for sort_program ({SORT_PROGRAM})"
-        "Valid values are 'none', 'samtools' or 'picard'"
+        f"Invalid sort_program '{SORT_PROGRAM}'. Valid values are: 'none', 'samtools' or 'picard'"
     )

Also applies to: 112-116

---

150-153: Simplify conditional logic using ternary operator

The conditional logic for interleaved input can be simplified.

-        if IS_INTERLEAVED:
-            cmd_input = f"--interleaved {SAMPLE}"
-        else:
-            cmd_input = f"-U {SAMPLE}"
+        cmd_input = f"--interleaved {SAMPLE}" if IS_INTERLEAVED else f"-U {SAMPLE}"

🧰 Tools

🪛 Ruff (0.8.2)

150-153: Use ternary operator cmd_input = f"--interleaved {SAMPLE}" if IS_INTERLEAVED else f"-U {SAMPLE}" instead of if-else-block

Replace if-else-block with cmd_input = f"--interleaved {SAMPLE}" if IS_INTERLEAVED else f"-U {SAMPLE}"

(SIM108)

---

194-194: Remove unnecessary f-string prefixes

Some strings are marked as f-strings but don't contain any placeholders.

-            SAMTOOLS_OPTS += f"--write-index "
+            SAMTOOLS_OPTS += "--write-index "
-            PICARD_OPTS += f"--CREATE_INDEX true "
+            PICARD_OPTS += "--CREATE_INDEX true "

Also applies to: 214-214

🧰 Tools

🪛 Ruff (0.8.2)

194-194: f-string without any placeholders

Remove extraneous f prefix

(F541)

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

bio/bowtie2/align/wrapper.py (3)

34-38: Consider removing commented-out output variables.

These commented-out variables (METRICS, UNALIGNED, etc.) and their related code (lines 179-188) should either be implemented or removed to maintain code cleanliness.

Would you like me to help clean up these commented sections?

---

197-198: Simplify thread check condition.

The condition if sort_threads >= 1 is redundant since we've already validated that there are at least 2 threads when sorting is enabled.

-        if sort_threads >= 1:
-            SAMTOOLS_OPTS += f"--threads {sort_threads} "
+        SAMTOOLS_OPTS += f"--threads {sort_threads} "

---

204-205: Make SORT_EXTRA modification more explicit.

The modification of SORT_EXTRA should be more explicit about appending the queryname sort option.

-        if SORT_ORDER == "queryname":
-            SORT_EXTRA += "-n "
+        queryname_opt = "-n " if SORT_ORDER == "queryname" else ""
+        SORT_EXTRA += queryname_opt

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

#!/bin/bash
# Description: Verify sorting implementation and its usage across the codebase

# Check for other wrappers using similar sorting logic
echo "Checking for similar sorting implementations:"
rg -A 5 'sort_program.*=.*("samtools"|"picard")'

# Check for test cases covering sorting functionality
echo "Checking for sort-related test cases:"
rg -A 10 'sort_program.*=.*(samtools|picard)' test/

# Check for potential issues with sort order handling
echo "Checking sort order handling:"
rg -A 5 'sort_order.*=.*("coordinate"|"queryname")'

bio/bowtie2/align/wrapper.py (4)

16-23: Add input validation to get_extension function

The function should validate that the input filename is not empty and is a string.

 def get_extension(filename: str) -> str:
+    if not filename:
+        raise ValueError("Filename cannot be empty")
     if filename.endswith((".gz", ".bz2")):
         return filename.split(".")[-2].lower()
     return filename.split(".")[-1].lower()

---

34-38: Clean up or document commented code

Several output variables are commented out. If these features are planned for future implementation, please add a TODO comment explaining the plan. If they are no longer needed, consider removing them.

---

108-112: Improve error message formatting

The error message for invalid sort_order is missing proper punctuation.

     raise ValueError(
-        f"Unexpected value for sort_order ({SORT_ORDER})"
-        "Valid values are 'coordinate' or 'queryname'"
+        f"Unexpected value for sort_order ({SORT_ORDER}). "
+        "Valid values are 'coordinate' or 'queryname'."
     )

---

187-235: Add error handling for invalid sort program combinations

While the validation checks prevent invalid combinations, it would be good practice to add an explicit error case in the pattern matching.

     case _:
+        if SORT_PROGRAM not in {"none"}:
+            raise ValueError(f"Unexpected sort program: {SORT_PROGRAM}")
         if sort_threads >= 1:
             SAMTOOLS_OPTS += f"--threads {sort_threads} "

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

bio/bowtie2/align/wrapper.py (5)

16-23: Enhance extension handling robustness

The extension handling could be improved to be more resilient:

• Case-insensitive extension comparison
• Handle paths with multiple dots

Consider this improved implementation:

 def get_extension(filename: str) -> str:
     """
     Return file format since Bowtie2 reads files that
     could be gzip'ed (extension: .gz) or bzip2'ed (extension: .bz2).
     """
-    if filename.endswith((".gz", ".bz2")):
-        return filename.split(".")[-2].lower()
-    return filename.split(".")[-1].lower()
+    filename = filename.lower()
+    base, ext = path.splitext(filename)
+    if ext in ('.gz', '.bz2'):
+        return path.splitext(base)[1][1:]  # Remove leading dot
+    return ext[1:]  # Remove leading dot

---

34-38: Document rationale for commented-out output variables

These commented-out variables suggest planned functionality for metrics and different alignment outputs. Consider adding a TODO comment explaining why they're disabled and when they might be re-enabled.

Add a comment explaining the rationale:

+ # TODO: These outputs are temporarily disabled due to complexity with SE/PE handling
+ # They can be re-enabled once we implement proper SE/PE output handling
 # METRICS = snakemake.output.get("metrics", None)
 # UNALIGNED = snakemake.output.get("unaligned", None)
 # UNPAIRED = snakemake.output.get("unpaired", None)
 # UNCONCORDANT = snakemake.output.get("unconcordant", None)
 # CONCORDANT = snakemake.output.get("concordant", None)

---

64-68: Enhance error message with actual input details

The error message could be more helpful by including the actual input received.

 if not isinstance(SAMPLE, str) and len(SAMPLE) not in [1, 2]:
     raise ValueError(
         "Input must have 1 (single-end) or 2 (paired-end) elements, "
-        f"got {len(SAMPLE)} elements"
+        f"got {len(SAMPLE)} elements: {SAMPLE}"
     )

---

91-101: Standardize error message formatting

The error messages for output validation should follow a consistent format.

 if bam_extension.lower() not in {"sam", "bam", "cram"}:
     raise ValueError(
-        f"Unrecognized extension for output file: {bam_extension}."
-        "Valid extensions are 'sam', 'bam' or 'cram'"
+        f"Unrecognized extension for output file: {bam_extension}. "
+        "Valid extensions are: 'sam', 'bam' or 'cram'"
     )

 if bai_extension not in {None, "bai", "crai"}:
     raise ValueError(
-        f"Unrecognized extension for index file: {bai_extension}."
-        "Valid extensions are 'bai' or 'crai'"
+        f"Unrecognized extension for index file: {bai_extension}. "
+        "Valid extensions are: 'bai' or 'crai'"
     )

---

200-206: Use consistent string formatting style

The command string uses inconsistent formatting styles. Consider using f-strings consistently.

         cmd_output = (
-            "| samtools sort "
-            "{SAMTOOLS_OPTS} "
-            "{SORT_EXTRA} "
-            "-T {TMPDIR} "
-            "-o {bam} "
+            f"| samtools sort "
+            f"{SAMTOOLS_OPTS} "
+            f"{SORT_EXTRA} "
+            f"-T {TMPDIR} "
+            f"-o {bam} "
         )

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

bio/bowtie2/align/wrapper.py (4)

16-25: Add input validation to get_extension

The function should validate that the input is not None or empty to prevent potential runtime errors.

 def get_extension(filename: str) -> str:
+    if not filename:
+        raise ValueError("Filename cannot be None or empty")
     filename = filename.lower()
     base, ext = path.splitext(filename)

---

36-42: Consider documenting the plan for re-enabling outputs

The TODO comment indicates that several outputs are temporarily disabled due to SE/PE handling complexity. Consider:

1. Creating a tracking issue for re-enabling these outputs
2. Documenting the specific challenges with SE/PE handling
3. Adding acceptance criteria for when these can be re-enabled

Would you like me to help create a GitHub issue to track this enhancement?

---

112-116: Improve error message formatting

The error message for invalid sort order is missing a space between the message parts.

     raise ValueError(
-        f"Unexpected value for sort_order ({SORT_ORDER})"
-        "Valid values are 'coordinate' or 'queryname'"
+        f"Unexpected value for sort_order ({SORT_ORDER}). "
+        "Valid values are 'coordinate' or 'queryname'"
     )

---

193-210: Consider extracting samtools command construction

The samtools command construction logic is complex and could benefit from being extracted into a helper function for better maintainability.

+def build_samtools_sort_command(bam, bai, sort_order, ref, sort_threads):
+    opts = get_samtools_opts(snakemake, parse_threads=False, param_name="sort_extra")
+    opts += f" --threads {sort_threads} "
+    if bai:
+        opts += "--write-index "
+    if sort_order == "queryname":
+        opts += "-n "
+    if get_extension(bam) == "cram":
+        opts += f"--reference {ref} --output-fmt CRAM "
+    return (
+        "| samtools sort "
+        f"{opts} "
+        "{SORT_EXTRA} "
+        "-T {TMPDIR} "
+        f"-o {bam} "
+    )

 match SORT_PROGRAM:
     case "samtools":
-        SAMTOOLS_OPTS += f"--threads {sort_threads} "
-        if BAI:
-            bam = f"{BAM}##idx##{BAI}"
-            SAMTOOLS_OPTS += "--write-index "
-        else:
-            bam = BAM
-        if SORT_ORDER == "queryname":
-            SORT_EXTRA += "-n "
-        if bam_extension == "cram":
-            SAMTOOLS_OPTS += f"--reference {REF} --output-fmt CRAM "
-        cmd_output = (
-            "| samtools sort "
-            "{SAMTOOLS_OPTS} "
-            "{SORT_EXTRA} "
-            "-T {TMPDIR} "
-            "-o {bam} "
-        )
+        output_bam = f"{BAM}##idx##{BAI}" if BAI else BAM
+        cmd_output = build_samtools_sort_command(
+            output_bam, BAI, SORT_ORDER, REF, sort_threads
+        )