sprocket format overwrite the repo by a-frantz · Pull Request #303 · stjudecloud/workflows

a-frantz · 2026-02-25T16:17:17Z

Describe the problem or feature in addition to a link to the issues.

Before submitting this PR, please make sure:

You have added a few sentences describing the PR here.
The code passes all CI tests without any errors or warnings.
You have added tests (when appropriate).
You have added an entry in any relevant CHANGELOGs (when appropriate).
If you have made any changes to the scripts/ or docker/ directories, please ensure any image versions have been incremented accordingly!
You have updated the README or other documentation to account for these changes (when appropriate).

github-advanced-security · 2026-03-02T09:37:34Z

This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation.

adthrasher

All of these comments are based on the first commit.

adthrasher · 2026-03-02T14:17:40Z

tools/arriba.wdl

+    Int disk_size_gb = bam_size_gb + ceil(size(gtf, "GB")) + ceil(size(reference_fasta_gz,
+        "GB")) + modify_disk_size_gb


This isn't that bad, but I'd almost prefer to treat addition chains similarly to arrays and line break on each element. This line break makes sense to me as it is the first available break that doesn't exceed the line length, but it feels weird to break in the middle of the size call inside of a ceil and within the + operator.

maybe length of chain should be considered? Something like "if more than N (3? 4?) binary operators in a chain, line break on each"?

I think this comment is superseded by the "priority list" and "clauses" discussion we've had.

adthrasher · 2026-03-02T14:18:19Z

tools/arriba.wdl

-            ~{(
-                if length(interesting_contigs) > 0
-                then "-i " + sep(",", quote(interesting_contigs))
-                else ""
-            )} \
-            ~{(
-                if length(viral_contigs) > 0
-                then "-v " + sep(",", quote(viral_contigs))
-                else ""
-            )} \
-            ~{(
-                if length(disable_filters) > 0
-                then "-f " + sep(",", quote(disable_filters))
-                else ""
-            )} \
+            ~{(if length(interesting_contigs) > 0 then "-i " + sep(",", quote(
+                interesting_contigs
+            )) else "")} \
+            ~{(if length(viral_contigs) > 0 then "-v " + sep(",", quote(viral_contigs))
+                else "")} \
+            ~{(if length(disable_filters) > 0 then "-f " + sep(",", quote(disable_filters)
+                ) else "")} \


Similar thoughts here.

adthrasher · 2026-03-02T14:19:17Z

tools/bwa.wdl

-        String prefix = sub(
-            basename(fastq),
-            "(([_.][rR](?:ead)?[12])((?:[_.-][^_.-]*?)*?))?\\.(fastq|fq)(\\.gz)?$",
+        String prefix = sub(basename(fastq), "(([_.][rR](?:ead)?[12])((?:[_.-][^_.-]*?)*?))?\\.(fastq|fq)(\\.gz)?$",
            ""  # Once replacing with capturing groups is supported, replace with group 3
-        )
+            )


adthrasher · 2026-03-02T14:19:38Z

tools/bwa.wdl

+    Int disk_size_gb = (ceil((input_fastq_size + reference_size) * 2) + 10 + modify_disk_size_gb
+        )


This is a really unfortunate line break.

adthrasher · 2026-03-02T14:21:58Z

tools/gatk4.wdl

+            --java-options "-XX:GCTimeLimit=50 -XX:GCHeapFreeLimit=10 -Xms4000m -Xmx~{
+                java_heap_size
+            }g" \


This reads awkwardly to me with the line breaks.

adthrasher · 2026-03-02T14:23:00Z

tools/kraken2.wdl

+    Int disk_size_gb = ((if library_name == "bacteria" then 300 else if library_name == "nr"
+        then 600 else if library_name == "nt" then 2500 else 25) + modify_disk_size_gb)


This impacts readability.

adthrasher · 2026-03-02T14:35:40Z

workflows/methylation/methylation-standard.wdl

+                Int num = (if merge_num > 0 then probe_num + (merge_num * max_length) else probe_num
+                    )


This is also an awkward break.

adthrasher · 2026-03-02T14:38:35Z

workflows/reference/qc-reference.wdl

+    if ((length(kraken_fastas) > 0) || (length(kraken_fasta_urls) > 0) || (length(
+        kraken_libraries
+    ) > 0)) {
+        call kraken2.download_taxonomy { input:
+            protein,
+        }


I wonder if we need to create a priority list of tokens for line breaking. Using this example, I'd prefer to backtrack and line break around || so that the entire clause (length(kraken_libraries) > 0) remains on a single line.

adthrasher

Comments based on the second commit.

adthrasher · 2026-03-02T14:45:49Z

tools/bwa.wdl

    Int disk_size_gb = (ceil((input_fastq_size + reference_size) * 2) + 10 + modify_disk_size_gb
-        )
+    )


Removing the indents is good. It'd be great to avoid dangling closures though.

adthrasher · 2026-03-02T14:47:14Z

tools/star.wdl

            --clip3pAdapterMMp ~{clip_3p_adapter_mmp.left} ~{(if (length(read_twos) != 0)
-             then clip_3p_adapter_mmp.right else None)} \
+                then clip_3p_adapter_mmp.right
+                else None)} \


This looks better to me.

a-frantz · 2026-03-02T15:26:41Z

tools/arriba.wdl

-            ~{if mark_duplicates then "" else "-u"} \
-            ~{if report_additional_columns then "-X" else ""} \
-            ~{if fill_gaps then "-I" else ""}
+            ~{if mark_duplicates
+                then ""
+                else "-u"
+            } \
+            ~{if report_additional_columns
+                then "-X"
+                else ""
+            } \
+            ~{if fill_gaps
+                then "-I"
+                else ""
+            }


I was worried about this change being too awkward for short clauses, but I think even this one looks pretty good here

Agreed. I think this is OK even for these short clauses and statements that could be single lines.

adthrasher · 2026-03-02T15:34:30Z

tools/star.wdl

            ]))} \
-            --clip3pAdapterSeq "~{clip_3p_adapter_seq.left}" ~{(if (length(read_twos) != 0
-            ) then "'" + clip_3p_adapter_seq.right + "'" else "")} \
+                --clip3pAdapterSeq "~{clip_3p_adapter_seq.left}" ~{(if (length(read_twos)


It looks like this line got an extra level of indent.

huh. That's a strange one. I don't have an immediate answer as to how this happened

oop. I did something that broke idempotency. There are extra indents being added each time I run this code 😅 This appears to be the only odd case in this commit, but I'm seeing some others locally

I think there was another instance down a few lines, but it sounds like you've got the cause figured out already.

a-frantz added 2 commits February 25, 2026 11:16

sprocket format overwrite the repo

494b151

format overwrite from PR branch

cea1fcd

stjude-rust-labs/sprocket#678

adthrasher reviewed Mar 2, 2026

View reviewed changes

multiline if else then clauses

177b605

a-frantz commented Mar 2, 2026

View reviewed changes

adthrasher reviewed Mar 2, 2026

View reviewed changes

a-frantz mentioned this pull request Mar 2, 2026

improve format line breaking stjude-rust-labs/sprocket#678

Draft

12 tasks

		Int disk_size_gb = bam_size_gb + ceil(size(gtf, "GB")) + ceil(size(reference_fasta_gz,
		"GB")) + modify_disk_size_gb

		Int disk_size_gb = (ceil((input_fastq_size + reference_size) * 2) + 10 + modify_disk_size_gb
		)

		Int disk_size_gb = ((if library_name == "bacteria" then 300 else if library_name == "nr"
		then 600 else if library_name == "nt" then 2500 else 25) + modify_disk_size_gb)

		Int num = (if merge_num > 0 then probe_num + (merge_num * max_length) else probe_num
		)

Conversation

a-frantz commented Feb 25, 2026

Uh oh!

github-advanced-security bot commented Mar 2, 2026

Uh oh!

adthrasher left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adthrasher left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants