Initial support for rendering wrapped, bulleted lists. #3576

mitchell-as · 2024-10-31T22:48:41Z

DX-2829 Failure messages remain indented with line breaks

Also do not put wrapped links on their own line. Links are not wrapped and indented because they would not be copy-pastable into browsers without manual removal of spaces and/or newlines.

Also do not put wrapped links on their own line.

Naatan

First review based on screenshot alone, I'll review the code once we get it looking right so we're not churning on code that's still gonna be changed.

The indented message seems to be missing one space
The indentation marks should not be given the color of the message (ie. they should remain black/default regardless of the message color).

I recognize the second point might require some significant business logic. Please move it back to triage if this creates a lot of additional work. This isn't worth a ton of investment, but we also shouldn't land the change as is since it's just trading one visual bug for another.

Naatan · 2024-11-05T17:43:53Z

internal/colorize/wrap_test.go

@@ -7,82 +7,82 @@ import (
 	"testing"
 )

-func Test_GetCroppedText(t *testing.T) {


Please add tests covering the new mechanic. Particularly color codes not covering the "continuation".

Naatan · 2024-11-05T17:46:16Z

internal/colorize/wrap.go

@@ -100,6 +115,19 @@ func inRange(pos int, ranges [][]int) bool {
 	return false
 }

+// colorTag returns the currently active color tag (if any) at the given position.
+func colorTag(pos int, ranges [][]int, names [][]string) string {


This assumes there's only ever one color tag, but we could have: [BOLD][RED], for example.

Naatan · 2024-11-05T17:46:42Z

internal/colorize/wrap.go

+					logging.Debug("continuation: '%s'", continuation)
+					logging.Debug("wrapped: '%s'", wrapped)


Suggested change

logging.Debug("continuation: '%s'", continuation)

logging.Debug("wrapped: '%s'", wrapped)

Naatan · 2024-11-05T17:48:43Z

internal/output/renderers/bulletlist.go

+	return &bulletList{prefix, items, bullets}
+}
+
+func (b *bulletList) MarshalOutput(format output.Format) interface{} {


Unless this creates some deep rabbit hole I'd prefer we don't address the magical output handling that we're slowly phasing out. So this could just be a String() function, or whatever you feel is appropriate so long as it's not trying to satisfy the output interface.

That also means that use-cases of this functionality will need to handle structured vs non-structured themselves. That part might be a rabbit hole, depending on the use-cases we have..

Also worth having a unit test for this.

Naatan · 2024-11-05T17:52:01Z

internal/output/renderers/bulletlist.go

+	if len(bullets) != 4 {
+		multilog.Error("Invalid bullet list: 4 bullets required")
+		bullets = BulletTree
+	}


Should return the error.

By making it typed as suggested later in the review, there cannot be an error to return anymore.

Naatan · 2024-11-05T17:52:24Z

internal/output/renderers/bulletlist.go

+// ├─ two
+// │ wrapped


Suggested change

// ├─ two

// │ wrapped

// ├─ two

// │ wrapped

Naatan · 2024-11-05T17:55:24Z

internal/output/renderers/bulletlist.go

+// ├─ two
+// │ wrapped
+// └─ three
+var BulletTree = []string{output.TreeMid, output.TreeMid, output.TreeLink + " ", output.TreeEnd}


Since we're using presets anyway, can we make this typed so it's easier to understand the code while reading?

eg.

type Bullets { Start string Mid string Link string End string }

Naatan · 2024-11-05T17:57:22Z

internal/runbits/dependencies/changesummary.go

@@ -130,8 +127,9 @@ func OutputChangeSummary(out output.Outputer, newBuildPlan *buildplan.BuildPlan,
 			item = fmt.Sprintf("[ACTIONABLE]%s@%s[/RESET] → %s (%s)", oldVersion.Name, oldVersion.Version, item, locale.Tl("updated", "updated"))
 		}

-		out.Notice(fmt.Sprintf("  [DISABLED]%s[/RESET] %s", prefix, item))


This seems to be dropping the [DISABLED].

There's a new BulletTreeDisabled preset.

Naatan · 2024-11-05T17:58:33Z

internal/runbits/dependencies/changesummary.go

 	}

+	out.Notice(renderers.NewBulletList("  ", renderers.BulletTree, items))


Fwiw, with regards to dropping the magical output handing, this could simply become:

out.Notice(renderers.NewBulletList(" ", renderers.BulletTree, items).String())

Naatan · 2024-11-05T17:59:30Z

internal/runbits/dependencies/summary.go

@@ -44,10 +41,9 @@ func OutputSummary(out output.Outputer, directDependencies buildplan.Artifacts)
 			subdepLocale = locale.Tl("summary_subdeps", "([ACTIONABLE]{{.V0}}[/RESET] sub-dependencies)", strconv.Itoa(numSubs))
 		}

-		item := fmt.Sprintf("[ACTIONABLE]%s@%s[/RESET] %s", ingredient.Name, ingredient.Version, subdepLocale)
-
-		out.Notice(fmt.Sprintf("[DISABLED]%s[/RESET] %s", prefix, item))


Why drop the DISABLED item here?

There's a new BulletTreeDisabled preset I'm using now.

…ed unit test.

…hod instead of output marshaler method.

mitchell-as · 2024-11-06T17:00:32Z

internal/colorize/wrap.go

+var indentRegexp = regexp.MustCompile(`^([ ]+)`)
+var isLinkRegexp = regexp.MustCompile(`\s*(\[[^\]]+\])?https?://`)
+
+func Wrap(text string, maxLen int, includeLineEnds bool, continuation string) WrappedLines {


Despite me doing a rename, GitHub thinks this entire file is new. I refactored this method and added colorTags() below. That's it. Everything else is the same.

mitchell-as · 2024-11-06T17:00:58Z

internal/colorize/wrap.go

+}
+
+// colorTags returns the currently active color tags (if any) at the given position.
+func colorTags(pos int, ranges [][]int, names [][]string) []string {


This is new after the rename.

mitchell-as · 2024-11-06T17:01:38Z

internal/colorize/wrap_test.go

+	}
+}
+
+func TestWrapBullet(t *testing.T) {


Despite renaming this file, GitHub thinks all of its contents are new. I added this test and the one that follows. The tests above I left alone.

Naatan

Nice work! Almost felt like I was reviewing a Komodo PR..

Initial support for rendering wrapped, bulleted lists.

48614a3

Also do not put wrapped links on their own line.

mitchell-as added Test: package Test: cve Test: checkout Test: artifacts labels Oct 31, 2024

mitchell-as requested a review from Naatan November 1, 2024 20:18

mitchell-as marked this pull request as ready for review November 1, 2024 20:19

Naatan requested changes Nov 1, 2024

View reviewed changes

mitchell-as requested a review from Naatan November 1, 2024 22:12

Do not colorize wrapping continuation characters.

55a07c0

mitchell-as force-pushed the mitchell/dx-2829-2 branch from 90b0b6c to 55a07c0 Compare November 4, 2024 15:26

Naatan requested changes Nov 5, 2024

View reviewed changes

mitchell-as added 2 commits November 6, 2024 11:54

Refactor wrapping, support multiple tags after continuations, and add…

3db6eb7

…ed unit test.

Use struct for bullet list bullets, added tests, and use String() met…

fe25e87

…hod instead of output marshaler method.

mitchell-as force-pushed the mitchell/dx-2829-2 branch from e52ce9c to fe25e87 Compare November 6, 2024 16:57

mitchell-as commented Nov 6, 2024

View reviewed changes

mitchell-as requested a review from Naatan November 6, 2024 17:16

Naatan approved these changes Nov 6, 2024

View reviewed changes

mitchell-as removed Test: package Test: cve Test: checkout Test: artifacts labels Nov 6, 2024

mitchell-as closed this Nov 6, 2024

mitchell-as reopened this Nov 6, 2024

mitchell-as merged commit fb1c600 into version/0-48-0-RC1 Nov 6, 2024
12 of 15 checks passed

mitchell-as deleted the mitchell/dx-2829-2 branch November 6, 2024 19:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial support for rendering wrapped, bulleted lists. #3576

Initial support for rendering wrapped, bulleted lists. #3576

mitchell-as commented Oct 31, 2024 •

edited by github-actions bot

Loading

Naatan left a comment

Naatan Nov 5, 2024

Naatan Nov 5, 2024

Naatan Nov 5, 2024

Naatan Nov 5, 2024

Naatan Nov 5, 2024

Naatan Nov 5, 2024

mitchell-as Nov 6, 2024

Naatan Nov 5, 2024

Naatan Nov 5, 2024

Naatan Nov 5, 2024

mitchell-as Nov 6, 2024

Naatan Nov 5, 2024

Naatan Nov 5, 2024

mitchell-as Nov 6, 2024

mitchell-as Nov 6, 2024

mitchell-as Nov 6, 2024

mitchell-as Nov 6, 2024

Naatan left a comment

		logging.Debug("continuation: '%s'", continuation)
		logging.Debug("wrapped: '%s'", wrapped)

		}

		out.Notice(renderers.NewBulletList(" ", renderers.BulletTree, items))

Initial support for rendering wrapped, bulleted lists. #3576

Initial support for rendering wrapped, bulleted lists. #3576

Conversation

mitchell-as commented Oct 31, 2024 • edited by github-actions bot Loading

Naatan 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

Naatan left a comment

Choose a reason for hiding this comment

mitchell-as commented Oct 31, 2024 •

edited by github-actions bot

Loading