-
Notifications
You must be signed in to change notification settings - Fork 458
console: swap supplementary-plane emoji for lightweight Unicode symbols #41578
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -150,7 +150,7 @@ func FormatSuccessMessage(message string) string { | |
|
|
||
| // FormatInfoMessage formats an informational message | ||
| func FormatInfoMessage(message string) string { | ||
| return applyStyle(styles.Info, "βΉ ") + message | ||
| return applyStyle(styles.Info, "i ") + message | ||
| } | ||
|
Comment on lines
151
to
154
|
||
|
|
||
| // FormatWarningMessage formats a warning message | ||
|
|
@@ -223,22 +223,22 @@ func RenderTable(config TableConfig) string { | |
|
|
||
| // FormatCommandMessage formats a command execution message | ||
| func FormatCommandMessage(command string) string { | ||
| return applyStyle(styles.Command, "β‘ ") + command | ||
| return applyStyle(styles.Command, "$ ") + command | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/zoom-out]
π‘ Convention conflict exampleIn gh-aw docs, shell examples look like: If |
||
| } | ||
|
Comment on lines
224
to
227
|
||
|
|
||
| // FormatProgressMessage formats a progress/activity message | ||
| func FormatProgressMessage(message string) string { | ||
| return applyStyle(styles.Progress, "π¨ ") + message | ||
| return applyStyle(styles.Progress, "βΈ ") + message | ||
| } | ||
|
|
||
| // FormatPromptMessage formats a user prompt message | ||
| func FormatPromptMessage(message string) string { | ||
| return applyStyle(styles.Prompt, "β ") + message | ||
| return applyStyle(styles.Prompt, "? ") + message | ||
| } | ||
|
|
||
| // FormatVerboseMessage formats verbose debugging output | ||
| func FormatVerboseMessage(message string) string { | ||
| return applyStyle(styles.Verbose, "π ") + message | ||
| return applyStyle(styles.Verbose, "Β» ") + message | ||
| } | ||
|
|
||
| // FormatListItem formats an item in a list | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -46,9 +46,9 @@ func TestFormatCommandMessage(t *testing.T) { | |
| t.Run(tt.name, func(t *testing.T) { | ||
| result := FormatCommandMessage(tt.command) | ||
|
|
||
| // Should contain the thunderbolt emoji prefix | ||
| if !strings.Contains(result, "β‘") { | ||
| t.Errorf("FormatCommandMessage() should contain β‘ prefix") | ||
| // Should contain the dollar-sign prefix | ||
| if !strings.Contains(result, "$") { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fragile prefix assertion: π‘ Suggested fixThe // Check the prefix is at the start of the unstyled output:
expected := "$ " + tt.command
if !strings.HasSuffix(result, tt.command) || !strings.Contains(result, "$") {
// ...
}
// Or better: strip styling and compare the full prefix form:
stripped := stripANSI(result)
if !strings.HasPrefix(stripped, "$ ") {
t.Errorf("FormatCommandMessage() should start with '$ ' prefix, got: %s", stripped)
}The chosen replacement symbol |
||
| t.Errorf("FormatCommandMessage() should contain $ prefix") | ||
| } | ||
|
|
||
| // Should contain the command text | ||
|
|
@@ -96,9 +96,9 @@ func TestFormatProgressMessage(t *testing.T) { | |
| t.Run(tt.name, func(t *testing.T) { | ||
| result := FormatProgressMessage(tt.message) | ||
|
|
||
| // Should contain the hammer emoji prefix | ||
| if !strings.Contains(result, "π¨") { | ||
| t.Errorf("FormatProgressMessage() should contain π¨ prefix") | ||
| // Should contain the triangle prefix | ||
| if !strings.Contains(result, "βΈ") { | ||
| t.Errorf("FormatProgressMessage() should contain βΈ prefix") | ||
| } | ||
|
|
||
| // Should contain the message text | ||
|
|
@@ -136,9 +136,9 @@ func TestFormatPromptMessage(t *testing.T) { | |
| t.Run(tt.name, func(t *testing.T) { | ||
| result := FormatPromptMessage(tt.message) | ||
|
|
||
| // Should contain the question mark emoji prefix | ||
| if !strings.Contains(result, "β") { | ||
| t.Errorf("FormatPromptMessage() should contain β prefix") | ||
| // Should contain the question mark prefix | ||
| if !strings.Contains(result, "?") { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Vacuous test assertion: π‘ Suggested fixAssert on a prefix-position check, not a substring anywhere. The simplest fix is to check that the prefix comes before the message, e.g.: // Instead of:
if !strings.Contains(result, "?")
// Verify the prefix is at the start (strip ANSI codes first if needed):
if !strings.HasPrefix(stripANSI(result), "? ") {
t.Errorf("FormatPromptMessage() should start with '? ' prefix")
}As written, the test passes even if |
||
| t.Errorf("FormatPromptMessage() should contain ? prefix") | ||
| } | ||
|
|
||
| // Should contain the message text | ||
|
|
@@ -176,9 +176,9 @@ func TestFormatVerboseMessage(t *testing.T) { | |
| t.Run(tt.name, func(t *testing.T) { | ||
| result := FormatVerboseMessage(tt.message) | ||
|
|
||
| // Should contain the magnifying glass emoji prefix | ||
| if !strings.Contains(result, "π") { | ||
| t.Errorf("FormatVerboseMessage() should contain π prefix") | ||
| // Should contain the double-angle prefix | ||
| if !strings.Contains(result, "Β»") { | ||
| t.Errorf("FormatVerboseMessage() should contain Β» prefix") | ||
| } | ||
|
|
||
| // Should contain the message text | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -174,7 +174,7 @@ func TestFormatInfoMessage(t *testing.T) { | |
| if !strings.Contains(output, "processing file") { | ||
| t.Errorf("Expected output to contain message, got: %s", output) | ||
| } | ||
| if !strings.Contains(output, "βΉ") { | ||
| if !strings.Contains(output, "i ") { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/tdd] The test now checks for Consider asserting on the full expected output or anchoring with
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Too-broad substring check: π‘ Suggested fixThe previous check for // Replace the weak substring check with a prefix check:
stripped := strings.TrimSpace(output)
if !strings.HasPrefix(stripped, "i ") {
t.Errorf("Expected output to start with info prefix 'i ', got: %s", output)
}Or update the golden file approach to capture the exact format. The current test would pass even if |
||
| t.Errorf("Expected output to contain info icon, got: %s", output) | ||
| } | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -65,15 +65,15 @@ func FormatError(err CompilerError) string { | |
| } | ||
|
|
||
| func FormatSuccessMessage(message string) string { return "β " + message } | ||
| func FormatInfoMessage(message string) string { return "βΉ " + message } | ||
| func FormatInfoMessage(message string) string { return "i " + message } | ||
| func FormatWarningMessage(message string) string { return "β " + message } | ||
|
Comment on lines
67
to
69
|
||
| func FormatErrorMessage(message string) string { return "β " + message } | ||
| func FormatLocationMessage(message string) string { return "π " + message } | ||
| func FormatCommandMessage(command string) string { return "β‘ " + command } | ||
| func FormatProgressMessage(message string) string { return "π¨ " + message } | ||
| func FormatPromptMessage(message string) string { return "β " + message } | ||
| func FormatCountMessage(message string) string { return "π " + message } | ||
| func FormatVerboseMessage(message string) string { return "π " + message } | ||
| func FormatLocationMessage(message string) string { return "~ " + message } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/zoom-out] Consider
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Misleading POSIX shorthand: π‘ Suggested fixConsider a neutral BMP symbol that communicates "location" without implying a home-directory relationship: // Options that don't carry POSIX home-dir meaning:
func FormatLocationMessage(message string) string { return "@ " + message } // common location marker
func FormatLocationMessage(message string) string { return "β " + message } // direction/target
func FormatLocationMessage(message string) string { return "β " + message } // U+2302, BMP house symbolA message like |
||
| func FormatCommandMessage(command string) string { return "$ " + command } | ||
| func FormatProgressMessage(message string) string { return "βΈ " + message } | ||
|
Comment on lines
+71
to
+73
|
||
| func FormatPromptMessage(message string) string { return "? " + message } | ||
| func FormatCountMessage(message string) string { return "# " + message } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/zoom-out] Alternatives:
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Overloaded symbol: π‘ Suggested fixIf the wasm output is ever consumed by scripts or embedded in Markdown, lines prefixed with // Less overloaded alternatives:
func FormatCountMessage(message string) string { return "n: " + message } // numeric label
func FormatCountMessage(message string) string { return "Ξ£ " + message } // U+03A3, BMP summation
func FormatCountMessage(message string) string { return "β’ " + message } // neutral bullet (already used for list items β keep distinct)The PR description's own criterion is "semantically equivalent", but |
||
| func FormatVerboseMessage(message string) string { return "Β» " + message } | ||
| func FormatListHeader(header string) string { return header } | ||
| func FormatListItem(item string) string { return " β’ " + item } | ||
| func FormatSectionHeader(header string) string { return header } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1 +1 @@ | ||
| β‘ gh aw compile workflow.md | ||
| $ gh aw compile workflow.md |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1 +1 @@ | ||
| βΉ Processing workflow file: test.md | ||
| i Processing workflow file: test.md |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1 +1 @@ | ||
| π Downloaded to: /tmp/logs/workflow-123 | ||
| ~ Downloaded to: /tmp/logs/workflow-123 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1 +1 @@ | ||
| π¨ Compiling workflow (step 2/5)... | ||
| βΈ Compiling workflow (step 2/5)... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[/zoom-out]
i(U+2139) is already a BMP character β yet the PR description explicitly lists it as an "already-lightweight" unchanged symbol while this line changes it to plaini.Replacing a visually distinct, well-supported BMP glyph with the letter
iweakens visual differentiation without addressing the stated compatibility goal (supplementary-plane emoji). Consider reverting this change, or update the PR description to accurately reflect that 8 symbols are swapped, not 6.π‘ PR description contradiction
The PR description states:
But
i(U+2139) IS changed here, andβ‘(U+26A1) is also changed inFormatCommandMessage. Neither is a supplementary-plane character. Updating the description table to include these two swaps would make the scope accurate.