feat(format): respect max line width #242

a-frantz · 2024-10-30T13:35:26Z

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

Took me a while to crack this problem, but with some help from @adthrasher I think we've finally got the feature working!

Still to do in a follow up PR: we want certain line breaks to be "matched", like if we break on one half of a parenthesis pair we should break on the other half as well. Until that gets implemented, some of the line breaks will look a little awkward.

Before submitting this PR, please make sure:

You have added a few sentences describing the PR here.
You have added yourself or the appropriate individual as the assignee.
You have added at least one relevant code reviewer to the PR.
Your code builds clean without any errors or warnings.
You have added tests (when appropriate).
You have updated the README or other documentation to account for these
changes (when appropriate).
You have added an entry to the relevant CHANGELOG.md (see
"keep a changelog" for more information).
Your commit messages follow the conventional commit style.

…/wdl into feat/line-width

adthrasher

Partial review before the interview starts

adthrasher · 2025-01-15T14:35:41Z

wdl-format/src/config/indent.rs


 /// An indentation level.
 #[derive(Clone, Copy, Debug)]
 pub enum Indent {
    /// Tabs.
-    Tabs(NonZeroUsize),
-
+    Tabs,


So if a user indents with tab, they can only use one tab. I'm fine with that, but just flagging that to make sure we're OK with that choice.

adthrasher · 2025-01-15T14:37:50Z

wdl-format/src/config/max_line_length.rs

+/// The default maximum line length.
+pub const DEFAULT_MAX_LINE_LENGTH: usize = 90;
+/// The minimum maximum line length.
+pub const MIN_MAX_LINE_LENGTH: usize = 60;
+/// The maximum maximum line length.
+pub const MAX_MAX_LINE_LENGTH: usize = 240;


Have you tested any non-default line lengths?

Check out these 3 commits on workflows - stjudecloud/workflows@main...tmp/format-examples

wdl-format/src/token.rs

adthrasher · 2025-01-15T14:44:32Z

wdl-format/src/token/post.rs

+    /// A temporary indent.
+    ///
+    /// This is added after a [`PostToken::Indent`] during the formatting of
+    /// command sections.


It's not clear to me from these comments why this is needed. Maybe there is additional documentation below where the command block is formatted, but it may be useful to expand here.

wdl-format/src/config/builder.rs

claymcleod · 2025-01-15T15:52:54Z

wdl-format/tests/format/ENCODE-DCC_chip-seq-pipeline/source.formatted.wdl

-            msg = "Cannot use automatic control subsampling (\"chip.ctl_depth_limit\">0 and \"chip.exp_ctl_depth_limit\">0) for " + "multiple controls with mixed endedness (e.g. SE ctl-rep1 and PE ctl-rep2). " + "Automatic control subsampling is enabled by default. " + "Disable automatic control subsampling by explicitly defining the above two parameters as 0 in your input JSON file. " + "You can still use manual control subsamping (\"chip.ctl_subsample_reads\">0) since it is done " + "for individual control's TAG-ALIGN output according to each control's endedness. ",
+            msg = "Cannot use automatic control subsampling (\"chip.ctl_depth_limit\">0 and \"chip.exp_ctl_depth_limit\">0) for "
+                + "multiple controls with mixed endedness (e.g. SE ctl-rep1 and PE ctl-rep2). "
+                + "Automatic control subsampling is enabled by default. " + "Disable automatic control subsampling by explicitly defining the above two parameters as 0 in your input JSON file. "


This feels like it should bring the second + down to the next line.

That would require backtracking, which is certainly possible but I'd say is OOS for this PR. I can open an issue for tracking that feature request.

FWIW, I missed the + in the middle of the line initially when trying to interpret Clay's comment.

peterhuene

Looks really good and I like the formatting changes, especially for if expressions.

Just a few comments.

wdl-format/src/config/builder.rs

wdl-format/src/config/max_line_length.rs

wdl-format/src/lib.rs

wdl-format/src/token/post.rs

claymcleod · 2025-01-15T15:54:19Z

wdl-format/tests/format/ENCODE-DCC_chip-seq-pipeline/source.formatted.wdl

-            ])
-        )
+        Boolean paired_end_ = if !defined(paired_end) && i < length(paired_ends) then paired_ends[
+            i] else select_first([


I'm on the fence whether I like this idea that ( and [ can split things down to the next line. For instance, here, I think it looks quite strange. I'm thinking that, for these lines that are being broken based on long line length, perhaps that should not be permitted.

I'm planning for a follow up PR that will line break "matching" tokens. So this example might become:

Boolean paired_end_ = if !defined(paired_end) && i < length(paired_ends) then paired_ends[ i ] else select_first([

or something similar. I haven't spent time thinking about the specifics yet, but I agree that the current implementation leaves something to be desired. I can push up a change the removes the open/close tokens from can_be_line_broken so we can see how it looks?

I feel like there should be some sort of priority list. Then if a line break is needed, you'd backtrack and find the highest "priority" line break location and break there. Here that's probably before then.

claymcleod · 2025-01-15T15:54:50Z

wdl-format/tests/format/ENCODE-DCC_chip-seq-pipeline/source.formatted.wdl

@@ -1627,7 +1472,8 @@ workflow chip {
            }
        }

-        Boolean has_input_of_count_signal_track = has_output_of_bam2ta || defined(bam2ta.ta)
+        Boolean has_input_of_count_signal_track = has_output_of_bam2ta || defined(bam2ta.ta
+            )


Similarly here, this is quite strange from a formatting perspective.

claymcleod · 2025-01-15T15:55:25Z

wdl-format/tests/format/ENCODE-DCC_chip-seq-pipeline/source.formatted.wdl

+        ])
+
+        Boolean has_input_of_align_ctl = i < length(ctl_fastqs_R1) && length(ctl_fastqs_R1[
+            i]) > 0


claymcleod · 2025-01-15T15:55:54Z

wdl-format/tests/format/ENCODE-DCC_chip-seq-pipeline/source.formatted.wdl


-        Boolean has_input_of_filter_ctl = has_output_of_align_ctl || defined(align_ctl.bam)
+        Boolean has_input_of_filter_ctl = has_output_of_align_ctl || defined(align_ctl.bam
+            )


And here. Sorry for so many comments, just want to call out places that require further examination.

wdl-format/src/token/pre.rs

adthrasher · 2025-01-15T17:50:05Z

wdl-format/tests/format/ENCODE-DCC_chip-seq-pipeline/source.formatted.wdl

+    File? blacklist_ = if length(blacklists) > 1 then pool_blacklist.ta_pooled else if length(
+        blacklists) > 0 then blacklists[0] else blacklist2_


I think this is one of the outstanding TODO items, but if not, we should revisit this case.

adthrasher · 2025-01-15T17:50:51Z

wdl-format/tests/format/ENCODE-DCC_chip-seq-pipeline/source.formatted.wdl

-        else "p.value"
-    )
+    String idr_rank_ = if peak_caller_ == "spp" then "signal.value" else if peak_caller_
+        == "macs2" then "p.value" else "p.value"


adthrasher · 2025-01-15T17:52:27Z

wdl-format/tests/format/ENCODE-DCC_chip-seq-pipeline/source.formatted.wdl

+    Int num_rep_nodup_bam = if length(nodup_bams) < num_rep_bam then num_rep_bam else length(
+        nodup_bams)
+    Int num_rep_ta = if length(tas) < num_rep_nodup_bam then num_rep_nodup_bam else length(
+        tas)


These are also cases to revisit.

adthrasher · 2025-01-15T17:52:39Z

wdl-format/tests/format/ENCODE-DCC_chip-seq-pipeline/source.formatted.wdl

+    Int num_ctl_bam = if length(ctl_bams) < num_ctl_fastq then num_ctl_fastq else length(
+        ctl_bams)
+    Int num_ctl_nodup_bam = if length(ctl_nodup_bams) < num_ctl_bam then num_ctl_bam else length(
+        ctl_nodup_bams)
+    Int num_ctl_ta = if length(ctl_tas) < num_ctl_nodup_bam then num_ctl_nodup_bam else length(
+        ctl_tas)


adthrasher · 2025-01-15T18:00:09Z

wdl-format/tests/format/ENCODE-DCC_chip-seq-pipeline/source.formatted.wdl

+        Boolean has_input_of_bam2ta_no_dedup = (has_output_of_align || defined(align.bam))
+            && !defined(bam2ta_no_dedup_R1.ta)


This is an example (in my opinion) of a good break. It broke on the && between clauses which would be my preference.

adthrasher · 2025-01-15T18:01:29Z

wdl-format/tests/format/ENCODE-DCC_chip-seq-pipeline/source.formatted.wdl

+    if (has_input_of_count_signal_track_pooled && enable_count_signal_track_ && num_rep > 1
+        ) {


Here's another case where it should probably backtrack and break on &&.

adthrasher · 2025-01-15T18:02:13Z

wdl-format/tests/format/ENCODE-DCC_chip-seq-pipeline/source.formatted.wdl

+        Int chosen_ctl_ta_id = if has_all_input_of_choose_ctl && !align_only_ then select_first(
+        [
+            choose_ctl.chosen_ctl_ta_ids,
+        ])[i] else -2


To me, this is a really awkward break.

adthrasher · 2025-01-15T18:03:22Z

wdl-format/tests/format/ENCODE-DCC_chip-seq-pipeline/source.formatted.wdl

+    Array[File?] chosen_ctl_ta_pooled = if !has_all_input_of_choose_ctl || align_only_
+        then [] else if chosen_ctl_ta_pooled_subsample > 0 then [
+        subsample_ctl_pooled.ta_subsampled,
+    ] else if num_ctl < 2 then [
+        ctl_ta_[0],
+    ] else [
+        pool_ta_ctl.ta_pooled,
+    ]


Is this indentation correct? It looks like an indent is dropped for the last 5 lines.

Here's the unformatted source:

Array[File?] chosen_ctl_ta_pooled = if !has_all_input_of_choose_ctl || align_only_ then [] else if chosen_ctl_ta_pooled_subsample > 0 then [ subsample_ctl_pooled.ta_subsampled ] else if num_ctl < 2 then [ ctl_ta_[0] ] else [ pool_ta_ctl.ta_pooled ]

adthrasher · 2025-01-15T18:09:15Z

wdl-format/tests/format/ENCODE-DCC_chip-seq-pipeline/source.formatted.wdl

@@ -3344,7 +2954,7 @@ task call_peak {
            ~{"--chrsz " + chrsz} \
            ~{"--fraglen " + fraglen} \
            ~{"--peak-type " + peak_type} \
-            ~{"--blacklist " + blacklist}
+            ~{"--blacklist " + blacklist}        


Why does this have trailing whitespace?

It's present in the unformatted file. I'm not sure why the prior implementation was stripping it 🤔 but I think it's supposed to be there.

wdl-format/src/config/builder.rs

peterhuene

Approved, but we should probably create a tracking issue for any follow-up tweaking needed to the line breaking output.

a-frantz added 10 commits October 22, 2024 13:56

[WIP] match current tests while using a buffer

e27f90c

[WIP]

f74d717

Update builder.rs

164261a

feat: config max line length

0f691d7

Update max_line_length.rs

c364a45

revise: indentation config

2852d28

Update post.rs

165603a

Merge branch 'main' into feat/line-width

f1f4dec

Merge branch 'main' into feat/line-width

f2d6448

WIP

ccc1104

claymcleod force-pushed the main branch from 4d16b8b to 0ff2852 Compare November 12, 2024 20:18

peterhuene force-pushed the main branch from 0ff2852 to 3b5aec2 Compare November 12, 2024 21:35

a-frantz added 18 commits January 2, 2025 10:59

Merge branch 'main' into feat/line-width

e1898e5

WIP

af36d91

revise: use constants for min and max max line len

ce3a454

revise: use a constant for max space indent

3278597

revise: hardcode tabs and spaces in fewer places

95d649c

chore: code clean up

163237e

WIP

283ae6a

WIP

df6f7a2

WIP

a238b95

chore: consistency

a574f09

Merge branch 'main' into feat/line-width

bc9f288

chore: code cleanup

caa29c8

revise: new PostToken (TempIndent)

7509e66

fix: handle edge case where stream is empty

3dc09c6

WIP

7c0e191

perf: use Rc for temp_indent

259ea59

WIP

a34c9e0

chore: fmt

7199199

Merge branch 'feat/line-width' of https://github.com/stjude-rust-labs…

d2c683e

…/wdl into feat/line-width

a-frantz self-assigned this Jan 15, 2025

a-frantz requested review from peterhuene, adthrasher and claymcleod January 15, 2025 12:15

Update CHANGELOG.md

d16da2a

a-frantz marked this pull request as ready for review January 15, 2025 12:18

adthrasher reviewed Jan 15, 2025

View reviewed changes

claymcleod reviewed Jan 15, 2025

View reviewed changes

wdl-format/src/config/builder.rs Outdated Show resolved Hide resolved

claymcleod reviewed Jan 15, 2025

View reviewed changes

wdl-format/src/config/builder.rs Show resolved Hide resolved

claymcleod reviewed Jan 15, 2025

View reviewed changes

peterhuene reviewed Jan 15, 2025

View reviewed changes

claymcleod reviewed Jan 15, 2025

View reviewed changes

a-frantz added 5 commits January 15, 2025 12:20

fix: rework Config Builder

b4d1020

Update builder.rs

f55d392

chore: review feedback

52a9984

Update wdl-format/src/token.rs

5a42020

revise: more specific name for line spacing policy

c0e446d

adthrasher reviewed Jan 15, 2025

View reviewed changes

chore: review feedback

b3a2631

peterhuene reviewed Jan 15, 2025

View reviewed changes

wdl-format/src/config/builder.rs Outdated Show resolved Hide resolved

a-frantz added 2 commits January 15, 2025 13:44

chore: review feedback

c62b9c1

chore: review feedback

1c2b6c7

peterhuene approved these changes Jan 15, 2025

View reviewed changes

a-frantz mentioned this pull request Jan 15, 2025

more sophisticated line breaking in wdl-format #293

Open

a-frantz merged commit dbe79da into main Jan 15, 2025
16 checks passed

a-frantz deleted the feat/line-width branch January 15, 2025 20:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(format): respect max line width #242

feat(format): respect max line width #242

a-frantz commented Oct 30, 2024 •

edited

Loading

adthrasher left a comment

adthrasher Jan 15, 2025

adthrasher Jan 15, 2025

a-frantz Jan 15, 2025

adthrasher Jan 15, 2025

claymcleod Jan 15, 2025

a-frantz Jan 15, 2025

adthrasher Jan 15, 2025

peterhuene left a comment

claymcleod Jan 15, 2025

a-frantz Jan 15, 2025

adthrasher Jan 15, 2025

claymcleod Jan 15, 2025

claymcleod Jan 15, 2025

claymcleod Jan 15, 2025

adthrasher Jan 15, 2025

adthrasher Jan 15, 2025

adthrasher Jan 15, 2025

adthrasher Jan 15, 2025

adthrasher Jan 15, 2025

adthrasher Jan 15, 2025

adthrasher Jan 15, 2025

adthrasher Jan 15, 2025

a-frantz Jan 15, 2025

adthrasher Jan 15, 2025

a-frantz Jan 15, 2025

peterhuene left a comment

		File? blacklist_ = if length(blacklists) > 1 then pool_blacklist.ta_pooled else if length(
		blacklists) > 0 then blacklists[0] else blacklist2_

		Boolean has_input_of_bam2ta_no_dedup = (has_output_of_align \|\| defined(align.bam))
		&& !defined(bam2ta_no_dedup_R1.ta)

		if (has_input_of_count_signal_track_pooled && enable_count_signal_track_ && num_rep > 1
		) {

feat(format): respect max line width #242

feat(format): respect max line width #242

Conversation

a-frantz commented Oct 30, 2024 • edited Loading

adthrasher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterhuene left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterhuene left a comment

Choose a reason for hiding this comment

a-frantz commented Oct 30, 2024 •

edited

Loading