Implement accessibility preferences: OutputPrefs and emoji prefixes#267
Open
Implement accessibility preferences: OutputPrefs and emoji prefixes#267
Conversation
- Introduce OutputPrefs struct to encapsulate emoji allowance and semantic prefix formatting. - Honor NO_COLOR and NETSUKE_NO_EMOJI environment variables and --no-emoji CLI flag. - Update CLI to include no_emoji option; integrate output preferences into status reporting. - Add localised semantic prefixes with and without emoji for Error, Warning, Success. - Provide comprehensive unit and BDD tests covering preference resolution and output. - Update documentation and roadmap to reflect new accessibility preference support. This completes accessibility preference support for Netsuke output, ensuring emoji can be suppressed and semantic prefixes remain consistent across modes. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
Contributor
Reviewer's GuideAdds a centralized OutputPrefs abstraction for emoji suppression and semantic prefixes, wires it through CLI parsing, status reporting, and main error handling, and updates localization, docs, and tests to cover accessibility preference behavior. Sequence diagram for status reporting with OutputPrefssequenceDiagram
actor User
participant Cli
participant Main as MainModule
participant Runner as RunnerModule
participant OutputPrefsMod as OutputPrefsModule
participant Reporter as AccessibleReporter
participant Stderr as StderrStream
User->>Cli: Invoke netsuke --accessible true --no-emoji true build
Cli-->>Main: Parsed Cli{accessible: Some(true), no_emoji: Some(true)}
Main->>OutputPrefsMod: resolve(merged_cli.no_emoji)
OutputPrefsMod-->>Main: OutputPrefs{emoji: false}
Main->>Runner: run(&merged_cli)
Runner->>OutputPrefsMod: resolve(cli.no_emoji)
OutputPrefsMod-->>Runner: OutputPrefs{emoji: false}
Runner->>Runner: mode = OutputMode::resolve(cli.accessible)
Runner->>Reporter: new(prefs)
Runner-->>Main: Ok(()) after build
loop build stages
Runner->>Reporter: report_stage(current, total, description)
Reporter->>Stderr: write Stage line
end
Runner->>Reporter: report_complete(tool_key)
Reporter->>OutputPrefsMod: success_prefix() on prefs
OutputPrefsMod-->>Reporter: LocalizedMessage "Success:" (no emoji)
Reporter->>Stderr: write "Success: {localized status complete message}"
Sequence diagram for error handling with semantic prefixessequenceDiagram
participant Main as MainModule
participant OutputPrefsMod as OutputPrefsModule
participant Runner as RunnerModule
participant Stderr as StderrStream
Main->>OutputPrefsMod: resolve(merged_cli.no_emoji)
OutputPrefsMod-->>Main: OutputPrefs prefs
Main->>Runner: run(&merged_cli)
Runner-->>Main: Err(err)
Main->>OutputPrefsMod: prefs.error_prefix()
OutputPrefsMod-->>Main: LocalizedMessage prefix
alt err is RunnerError
Main->>Main: report = Report::new(runner_err)
Main->>Stderr: write "{prefix} {report:?}"
else other error type
Main->>Stderr: write "{prefix} {other_err}"
end
Class diagram for OutputPrefs and related CLI/status structuresclassDiagram
class Cli {
+bool fetch_default_deny
+Option~bool~ accessible
+Option~bool~ no_emoji
+Option~Command~ command
}
class OutputPrefs {
-bool emoji
+bool emoji_allowed()
+LocalizedMessage error_prefix()
+LocalizedMessage warning_prefix()
+LocalizedMessage success_prefix()
+static OutputPrefs resolve(no_emoji: Option~bool~)
+static OutputPrefs resolve_with(no_emoji: Option~bool~, read_env: Fn~&str -> Option~String~~)
}
class StatusReporter {
<<trait>>
+report_stage(current: u32, total: u32, description: &str)
+report_pending(tool_key: &str)
+report_complete(tool_key: &'static str)
}
class AccessibleReporter {
-OutputPrefs prefs
+new(prefs: OutputPrefs) AccessibleReporter
+report_stage(current: u32, total: u32, description: &str)
+report_pending(tool_key: &str)
+report_complete(tool_key: &'static str)
}
class OutputMode {
<<enum>>
Accessible
Standard
+static resolve(accessible: Option~bool~) OutputMode
}
class RunnerModule {
+run(cli: &Cli) Result~()~
}
class MainModule {
+main() ExitCode
}
Cli --> OutputPrefs : no_emoji
MainModule ..> OutputPrefs : resolve(no_emoji)
RunnerModule ..> OutputMode : resolve(accessible)
RunnerModule ..> OutputPrefs : resolve(no_emoji)
RunnerModule ..> StatusReporter
AccessibleReporter ..|> StatusReporter
AccessibleReporter o--> OutputPrefs
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 Summary by CodeRabbit
WalkthroughIntroduce an output preference system that resolves emoji rendering from CLI (--no-emoji), environment (NO_COLOR, NETSUKE_NO_EMOJI) and configuration; expose Changes
Sequence DiagramsequenceDiagram
participant CLI as CLI Parser
participant Main as main()
participant OutputPrefs as output_prefs::resolve()
participant Env as Environment
participant Runner as runner::run()
participant Reporter as AccessibleReporter
participant Stderr as Stderr
CLI->>Main: provide merged_cli (includes no_emoji)
Main->>OutputPrefs: resolve(merged_cli.no_emoji)
OutputPrefs->>OutputPrefs: evaluate explicit CLI/config
alt explicit no_emoji = true
OutputPrefs->>OutputPrefs: disable emoji
else
OutputPrefs->>Env: read NO_COLOR
Env-->>OutputPrefs: value or none
alt NO_COLOR set
OutputPrefs->>OutputPrefs: disable emoji
else
OutputPrefs->>Env: read NETSUKE_NO_EMOJI
Env-->>OutputPrefs: value or none
alt NETSUKE_NO_EMOJI set
OutputPrefs->>OutputPrefs: disable emoji
else
OutputPrefs->>OutputPrefs: enable emoji (default)
end
end
end
OutputPrefs-->>Main: return prefs
Main->>Runner: run(cli, prefs)
Runner->>Reporter: AccessibleReporter::new(prefs)
Reporter->>Reporter: compute prefix via prefs.success_prefix()
Reporter-->>Stderr: write prefixed message
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Owner
Author
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Comment on lines +213 to +224 fn error_prefix_contains_error_text_without_emoji() {
let prefs = OutputPrefs { emoji: false };
let rendered = prefs.error_prefix().to_string();
assert!(
rendered.contains("Error:"),
"expected 'Error:' in '{rendered}'"
);
assert!(
rendered.is_ascii(),
"expected ASCII-only prefix, got '{rendered}'"
);
}❌ New issue: Code Duplication |
Owner
Author
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: tests/bdd/steps/accessibility_preferences.rs Comment on lines +101 to +108 fn render_error_prefix(world: &TestWorld) -> Result<()> {
let prefs = world
.output_prefs
.get()
.ok_or_else(|| anyhow::anyhow!("output prefs have not been resolved"))?;
world.rendered_prefix.set(prefs.error_prefix().to_string());
Ok(())
}❌ New issue: Code Duplication |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
…stest Replaced multiple similar unit tests for error, success, and warning prefixes with a single parameterized test using rstest. This reduces code duplication and improves test maintainability by covering both emoji and non-emoji cases in one test function. Additionally, in accessibility preference BDD steps, introduced a helper function to abstract prefix rendering logic, simplifying step definitions for rendering error and success prefixes. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4dd8914b64
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
src/output_prefs.rs
Outdated
Comment on lines
136
to
138
| if let Some(forced) = no_emoji { | ||
| return OutputPrefs { emoji: !forced }; | ||
| } |
There was a problem hiding this comment.
This early explicit-override branch causes a runtime regression when Cli.no_emoji is populated from environment merging (NETSUKE_ prefix in cli::merge_with_config): if a user exports NETSUKE_NO_EMOJI=false, it becomes Some(false) here and re-enables emoji, even though this resolver (and its docs/tests) define NETSUKE_NO_EMOJI as a presence-based signal where any set value should suppress emoji. In practice, templated env configs that emit boolean strings can now produce the opposite behaviour from the intended accessibility default.
Useful? React with 👍 / 👎.
Owner
Author
There was a problem hiding this comment.
@coderabbitai Has this now been resolved in the latest commit?
Use codegraph analysis to determine your answer.
If this comment is now resolved, please mark it as such using the API. Otherwise, please provide an AI agent prompt for the remaining work to be done to address this comment.
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the current implementation of resolve_with in src/output_prefs.rs
echo "=== Current resolve_with implementation ==="
ast-grep --pattern $'pub fn resolve_with<$_>($$$) $$$
where
$$$
{
$$$
}' src/output_prefs.rs
echo ""
echo "=== Checking for the problematic early return ==="
rg -A 5 'if let Some\(forced\) = no_emoji' src/output_prefs.rs
echo ""
echo "=== Check CLI merge logic for NETSUKE_NO_EMOJI handling ==="
rg -C 5 'NETSUKE_NO_EMOJI|no_emoji.*merge' src/cli/mod.rs
echo ""
echo "=== Check for any tests covering NETSUKE_NO_EMOJI=false ==="
rg -C 3 'NETSUKE_NO_EMOJI.*false|no_emoji.*false' tests/ src/
Length of output: 3971
🏁 Script executed:
#!/bin/bash
# Check how resolve is called in main.rs
echo "=== How resolve is called in main.rs ==="
rg -C 5 'output_prefs::resolve' src/main.rs
# Check for any environment variable merging in cli module
echo ""
echo "=== Check for environment variable merging with NETSUKE_ prefix ==="
rg -C 10 'merge_with|NETSUKE_' src/cli/mod.rs
# Check if there's any string to bool conversion for environment variables
echo ""
echo "=== Check for env var parsing to bool ==="
rg -C 5 'env::var.*parse|parse.*bool' src/
# Look for any config file parsing that might affect no_emoji
echo ""
echo "=== Check config module for no_emoji handling ==="
fd -e rs . src/ --exec rg -l 'no_emoji' {} \; | head -10
# Check the actual value passed to resolve in all callsites
echo ""
echo "=== All resolve callsites ==="
rg -C 3 'resolve\(' src/ tests/
Length of output: 11207
🏁 Script executed:
#!/bin/bash
# Check the full merge_with_config implementation to see environment layer handling
echo "=== Full merge_with_config function ==="
ast-grep --pattern $'pub fn merge_with_config($$$) -> $_ {
$$$
}' src/cli/mod.rs
# Look for environment layer or env prefix usage
echo ""
echo "=== Check for environment layer with NETSUKE_ prefix ==="
rg -C 10 'push_env|env_layer|ENV_PREFIX' src/cli/mod.rs
# Check Cli struct definition for serde attributes
echo ""
echo "=== Check Cli struct definition and attributes ==="
ast-grep --pattern 'pub struct Cli {
$$$
}' src/cli/mod.rs
# Check if ortho_config is in dependencies
echo ""
echo "=== Check Cargo.toml for ortho-config ==="
rg 'ortho' Cargo.toml
Length of output: 8202
@leynos, the issue has not been resolved in the latest commit.
The problematic code is still present at lines 136–138 of src/output_prefs.rs:
if let Some(forced) = no_emoji {
return OutputPrefs { emoji: !forced };
}Combined with the ortho-config environment merging at lines 369–378 of src/cli/mod.rs, the vulnerability remains: when a user sets NETSUKE_NO_EMOJI=false (or =0), Figment's Env provider parses it as boolean false, which becomes Some(false) in Cli.no_emoji. The early return then produces emoji: !false = true, re-enabling emoji and contradicting the documented presence-based semantics where any value should disable emoji.
Your tests explicitly cover this behaviour (explicit_emoji_on_overrides_netsuke_no_emoji), but the original concern was that environment-based boolean parsing undermines the intended "presence means suppress" contract for NETSUKE_NO_EMOJI.
AI agent prompt to address this issue:
Refactor `src/output_prefs.rs::resolve_with` to honour presence-based semantics for the `NETSUKE_NO_EMOJI` environment variable whilst preserving explicit CLI override behaviour. The function currently allows `no_emoji: Some(false)` to re-enable emoji unconditionally, but when this value originates from environment parsing (e.g., `NETSUKE_NO_EMOJI=false`), it violates the documented contract that any set value should suppress emoji.
Required changes:
1. Split the explicit override into two paths:
- `Some(true)` → force emoji off
- `Some(false)` → skip explicit override; proceed to environment checks
2. Update documentation and tests to reflect that `no_emoji: Some(false)` only acts as an override when explicitly set via CLI flag, not when derived from environment parsing.
3. Ensure that `NETSUKE_NO_EMOJI` (with any value, including "false" or "0") always suppresses emoji unless overridden by an explicit CLI `--no-emoji=false`.
4. Preserve existing test coverage for explicit CLI override behaviour (`explicit_emoji_on_overrides_no_color` and similar).
Maintain en-GB-oxendict spelling and adhere to the coding guidelines.
…nt output Refactor runner::run to accept output preferences, improving output handling consistency. Update main, tests, and relevant modules to pass resolved output preferences explicitly. Add BDD scenarios for warning prefix rendering in no-emoji mode. Adjust docs for wording fixes and terminology consistency. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@docs/execplans/3-8-2-respect-accessibility-preferences.md`:
- Around line 99-101: Update the British-variant -ise spellings to -ize in the
document: change "localised Fluent messages" to "localized Fluent messages" and
any other occurrences of "-ise" (e.g., in the semantic-prefix description that
uses `$emoji` select and the surrounding lines referenced) to their "-ize"
forms; apply the same replacement to the other affected occurrences noted (lines
108-110 and 117-118) so all instances use the en-GB-oxendict "-ize" spelling
consistently.
In `@src/status.rs`:
- Around line 33-35: Update the doc comment that currently reads "localised
messages from the Fluent resource bundle" to use en-GB-oxendict -ize spelling by
replacing "localised" with "localized"; locate the doc comment containing
"localised messages from the Fluent resource bundle" in src/status.rs (the
module/section documenting completion messages) and change the word only so the
rest of the comment remains unchanged.
Comment on lines
99
to
101
| - Semantic prefixes use localised Fluent messages with `$emoji` select. | ||
| - Emoji-allowed mode: `✖ Error:`, `⚠ Warning:`, `✔ Success:`. | ||
| No-emoji mode: `Error:`, `Warning:`, `Success:`. |
Contributor
There was a problem hiding this comment.
Replace -ise spellings with -ize forms.
Align these terms with en-GB-oxendict spelling.
Proposed fix
- Semantic prefixes use localised Fluent messages with `$emoji` select.
+ Semantic prefixes use localized Fluent messages with `$emoji` select.
@@
-New tests: 18 unit tests in `output_prefs::tests` (10 parameterised resolve
+New tests: 18 unit tests in `output_prefs::tests` (10 parameterized resolve
@@
-The `src/cli/mod.rs` 400-line risk materialised (387 + new field). Mitigated
+The `src/cli/mod.rs` 400-line risk materialized (387 + new field). MitigatedAs per coding guidelines, documentation must use en-GB-oxendict spelling with -ize forms.
Also applies to: 108-110, 117-118
🧰 Tools
🪛 LanguageTool
[style] ~99-~99: Would you like to use the Oxford spelling “localized”? The spelling ‘localised’ is also correct.
Context: ...ome(false)/on). - Semantic prefixes use localised Fluent messages with $emoji select. -...
(OXFORD_SPELLING_Z_NOT_S)
🤖 Prompt for AI Agents
In `@docs/execplans/3-8-2-respect-accessibility-preferences.md` around lines 99 -
101, Update the British-variant -ise spellings to -ize in the document: change
"localised Fluent messages" to "localized Fluent messages" and any other
occurrences of "-ise" (e.g., in the semantic-prefix description that uses
`$emoji` select and the surrounding lines referenced) to their "-ize" forms;
apply the same replacement to the other affected occurrences noted (lines
108-110 and 117-118) so all instances use the en-GB-oxendict "-ize" spelling
consistently.
Owner
Author
There was a problem hiding this comment.
@coderabbitai Has this now been resolved in the latest commit?
Use codegraph analysis to determine your answer.
If this comment is now resolved, please mark it as such using the API. Otherwise, please provide an AI agent prompt for the remaining work to be done to address this comment.
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Read the ExecPlan document and check for -ise vs -ize spellings
echo "=== Checking for -ise spellings that should be -ize ==="
cat docs/execplans/3-8-2-respect-accessibility-preferences.md | grep -n -E "(localised|parameterised|materialised)"
echo ""
echo "=== Checking for corrected -ize spellings ==="
cat docs/execplans/3-8-2-respect-accessibility-preferences.md | grep -n -E "(localized|parameterized|materialized)"
echo ""
echo "=== Showing lines around 99, 108-110, 117-118 ==="
sed -n '98,101p' docs/execplans/3-8-2-respect-accessibility-preferences.md
echo "---"
sed -n '107,111p' docs/execplans/3-8-2-respect-accessibility-preferences.md
echo "---"
sed -n '116,119p' docs/execplans/3-8-2-respect-accessibility-preferences.md
Length of output: 1078
@leynos, the comment has been partially resolved. The codegraph analysis shows:
Fixed:
- Line 100: "localised" → "localized" ✓
Still requires correction:
- Line 109: "parameterised" → needs "parameterized"
- Line 118: "materialised" → needs "materialized"
🤖 Prompt for AI Agents
In `@docs/execplans/3-8-2-respect-accessibility-preferences.md`, update the remaining British-variant -ise spellings to -ize forms: change "parameterised resolve" on line 109 to "parameterized resolve" and "materialised (387 + new field)" on line 118 to "materialized (387 + new field)" to align with en-GB-oxendict spelling.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@docs/execplans/3-8-2-respect-accessibility-preferences.md`:
- Around line 13-18: Update the semantic prefix list in the docs entry titled
"3-8-2-respect-accessibility-preferences.md" by inserting the Oxford comma so
the phrase reads "Error, Warning, and Success" instead of "Error, Warning,
Success"; locate the sentence containing "semantic prefixes (Error, Warning,
Success)" and edit it to include the comma before "and" to comply with the style
guideline.
In `@docs/users-guide.md`:
- Around line 625-628: Reflow the paragraph that describes semantic text
prefixes so it is wrapped at 80 columns and insert the Oxford comma when listing
the three prefixes — render them as `Error:`, `Warning:`, and `Success:` — and
ensure the sentence about including a leading glyph when emoji are permitted is
kept intact and grammatically correct while fitting the 80-column wrap.
Comment on lines
+13
to
+18
| Netsuke's accessible output mode (roadmap 3.8.1, complete) auto-detects | ||
| `NO_COLOR` and `TERM=dumb` to switch from animated to static status output. | ||
| However, the tool does not yet honour emoji preferences, enforce ASCII-only | ||
| output when requested, or guarantee semantic prefixes (Error, Warning, Success) | ||
| in every output mode. This plan completes the remaining accessibility | ||
| preferences so that: |
Contributor
There was a problem hiding this comment.
Add the Oxford comma to the semantic prefix list.
Write the list as “Error, Warning, and Success”.
Proposed diff
-However, the tool does not yet honour emoji preferences, enforce ASCII-only
-output when requested, or guarantee semantic prefixes (Error, Warning, Success)
-in every output mode. This plan completes the remaining accessibility
-preferences so that:
+However, the tool does not yet honour emoji preferences, enforce ASCII-only
+output when requested, or guarantee semantic prefixes (Error, Warning, and
+Success) in every output mode. This plan completes the remaining accessibility
+preferences so that:As per coding guidelines, "Use the Oxford comma in documentation: 'ships, planes, and hovercraft' where it aids comprehension."
🤖 Prompt for AI Agents
In `@docs/execplans/3-8-2-respect-accessibility-preferences.md` around lines 13 -
18, Update the semantic prefix list in the docs entry titled
"3-8-2-respect-accessibility-preferences.md" by inserting the Oxford comma so
the phrase reads "Error, Warning, and Success" instead of "Error, Warning,
Success"; locate the sentence containing "semantic prefixes (Error, Warning,
Success)" and edit it to include the comma before "and" to comply with the style
guideline.
Owner
Author
There was a problem hiding this comment.
@coderabbitai has this now been resolved?
Use sourcegraph analysis to determine your answer.
If the comment has been resolved, please mark it as resolved using the API. Otherwise, please provide an AI agent prompt for the work remaining.
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the current content of the file for the Oxford comma in the semantic prefix list
cat docs/execplans/3-8-2-respect-accessibility-preferences.md | grep -A2 -B2 "semantic prefixes"
Length of output: 322
@leynos, the issue has not been resolved. The file still contains "semantic prefixes (Error, Warning, Success)" without the Oxford comma.
🤖 AI Agent Prompt for Remaining Work
In the file `docs/execplans/3-8-2-respect-accessibility-preferences.md` at line 15, insert the Oxford comma before "and" in the phrase "semantic prefixes (Error, Warning, Success)" so it reads "semantic prefixes (Error, Warning, and Success)". Locate the sentence "However, the tool does not yet honour emoji preferences, enforce ASCII-only output when requested, or guarantee semantic prefixes (Error, Warning, Success) in every output mode." and update it to include the comma before "and Success)".
Comment on lines
+625
to
+628
| In all output modes, Netsuke uses semantic text prefixes (`Error:`, | ||
| `Warning:`, `Success:`) so that meaning is never conveyed solely by colour | ||
| or symbol. When emoji is permitted, these prefixes include a leading glyph | ||
| for quick visual scanning. |
Contributor
There was a problem hiding this comment.
Apply the Oxford comma and wrap the semantic-prefix paragraph to 80 columns.
Reflow the paragraph and render the three prefixes with an Oxford comma.
Proposed diff
-In all output modes, Netsuke uses semantic text prefixes (`Error:`,
-`Warning:`, `Success:`) so that meaning is never conveyed solely by colour
-or symbol. When emoji is permitted, these prefixes include a leading glyph
-for quick visual scanning.
+In all output modes, Netsuke uses semantic text prefixes (`Error:`,
+`Warning:`, and `Success:`) so that meaning is never conveyed solely
+by colour or symbol. When emoji is permitted, these prefixes include a
+leading glyph for quick visual scanning.As per coding guidelines, "Markdown paragraphs and bullet points must be wrapped at 80 columns" and "Use the Oxford comma in documentation: 'ships, planes, and hovercraft' where it aids comprehension."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| In all output modes, Netsuke uses semantic text prefixes (`Error:`, | |
| `Warning:`, `Success:`) so that meaning is never conveyed solely by colour | |
| or symbol. When emoji is permitted, these prefixes include a leading glyph | |
| for quick visual scanning. | |
| In all output modes, Netsuke uses semantic text prefixes (`Error:`, | |
| `Warning:`, and `Success:`) so that meaning is never conveyed solely | |
| by colour or symbol. When emoji is permitted, these prefixes include a | |
| leading glyph for quick visual scanning. |
🤖 Prompt for AI Agents
In `@docs/users-guide.md` around lines 625 - 628, Reflow the paragraph that
describes semantic text prefixes so it is wrapped at 80 columns and insert the
Oxford comma when listing the three prefixes — render them as `Error:`,
`Warning:`, and `Success:` — and ensure the sentence about including a leading
glyph when emoji are permitted is kept intact and grammatically correct while
fitting the 80-column wrap.
Owner
Author
There was a problem hiding this comment.
@coderabbitai has this now been resolved?
Use sourcegraph analysis to determine your answer.
If the comment has been resolved, please mark it as resolved using the API. Otherwise, please provide an AI agent prompt for the work remaining.
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Read the specific lines from the users guide to check if the review comment has been addressed
sed -n '625,628p' docs/users-guide.md
Length of output: 244
@leynos, the review comment has not been resolved.
The current text at lines 625-628 still reads:
In all output modes, Netsuke uses semantic text prefixes (`Error:`,
`Warning:`, `Success:`) so that meaning is never conveyed solely by colour
or symbol.
The Oxford comma is still missing (should be "Error:, Warning:, and Success:"), and the paragraph wrapping doesn't follow the 80-column guideline.
🤖 AI Agent Prompt for remaining work
In the file `docs/users-guide.md` at lines 625-628, update the paragraph
describing semantic text prefixes to:
1. Add the Oxford comma before the final item in the list, changing
"`Error:`, `Warning:`, `Success:`" to "`Error:`, `Warning:`, and `Success:`"
2. Reflow the entire paragraph to wrap at 80 columns while maintaining
grammatical correctness and readability
The corrected paragraph should read:
"In all output modes, Netsuke uses semantic text prefixes (`Error:`,
`Warning:`, and `Success:`) so that meaning is never conveyed solely
by colour or symbol. When emoji is permitted, these prefixes include a
leading glyph for quick visual scanning."
Owner
Author
|
@coderabbitai Have the following now been resolved? |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 2557 🏁 Script executed: Length of output: 358 🏁 Script executed: Length of output: 76 🏁 Script executed: Length of output: 1413 🏁 Script executed: Length of output: 229
Overall Comments ✅1. Single OutputPrefs instance passed to runner:
2. Retain tracing::error! alongside stderr output:
Individual Comments ✅Comment 1 – BDD coverage for warning prefix:
Comment 2 – "Emoji are" (plural):
Comment 3 – Expand "BDD" acronym:
Comment 4 – "localized" (not "localised"):
All issues have been addressed! 🎉 |
The output preference resolution logic was updated so that an explicit no_emoji value of Some(false) no longer forces emoji on unconditionally. Instead, Some(false) defers to environment variables like NO_COLOR and NETSUKE_NO_EMOJI, both of which use presence-based semantics to suppress emoji output. Only Some(true) forces emoji suppression unconditionally. This change ensures that setting --no-emoji false does not override environment variables that suppress emoji, addressing user expectations and aligning cli behavior with documented semantics. Updates include documentation, unit tests, BDD scenarios, and examples. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
leynos
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Changes
Core functionality
“✖ Error: …” vs “Error: …”).
CLI and localization
Documentation
Tests
Files touched
How to test
Migration and compatibility
Risks and notes
If you want, I can adjust the PR description to emphasize particular user-facing benefits or tailor the test matrix for CI.
◳ Generated by DevBoxer ◰
📎 Task: https://www.devboxer.com/task/79254424-c0da-4f85-8b04-4116d4aa87d1