Change some Ansi-code features. #2297
Conversation
Makes `AnsiCode.toString()` return `AnsiCode.escape`, so the code
can be embedded in strings directly like `"a ${red}red${resetAll} word"`.
Makes `AnsiCodeType` an `enum`. _May affect code that has an non-exhaustive
switch over `AnsiCodeType`._
This changes the `toString` of the enum values from `AnsiCode.<name>` to
`AnsiCodeType.<name>`.
Makes the `wrapWith` use the reset codes for the Ansi _style_ codes used,
instead of always using `resetAll`. If any color is included, it still uses
`resetAll`.
_May affect code that relied on `wrapWith` resetting more than its own
styles._
Package publishing
Documentation at https://github.com/dart-lang/ecosystem/wiki/Publishing-automation. |
PR HealthLicense Headers ✔️
All source files should start with a license header. Unrelated files missing license headers
This check can be disabled by tagging the PR with API leaks ✔️The following packages contain symbols visible in the public API, but not exported by the library. Export these symbols or remove them from your publicly visible API.
This check can be disabled by tagging the PR with Breaking changes ✔️
This check can be disabled by tagging the PR with Coverage ✔️
This check for test coverage is informational (issues shown here will not fail the PR). This check can be disabled by tagging the PR with Changelog Entry ✔️
Changes to files need to be accounted for in their respective changelogs. This check can be disabled by tagging the PR with |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces several valuable changes to the ANSI code handling, including making AnsiCodeType an enum, changing AnsiCode.toString() to return the escape sequence for easier string embedding, and refining wrapWith to use specific reset codes for styles. These are great improvements for usability and correctness. The code is well-structured, and the accompanying test changes are thorough. I have one suggestion to simplify the logic for collecting reset codes in wrapWith, which could make the implementation more declarative and easier to read.
| final resets = <AnsiCode>{}; // Include each reset only once. | ||
| for (var code in sortedCodes.reversed) { | ||
| final resetCode = code.reset!; | ||
| if (!identical(resetCode, resetAll)) { | ||
| resets.add(resetCode); | ||
| } else { | ||
| resets | ||
| ..clear() | ||
| ..add(resetAll); | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
This logic for determining the set of reset codes can be expressed more concisely and declaratively. The current loop is a bit complex. You can achieve the same result by first checking if any code requires a full reset, and then collecting the appropriate reset codes based on that condition.
final resets = myCodes.any((c) => identical(c.reset, resetAll))
? {resetAll}
: myCodes.map((c) => c.reset!).toSet();There was a problem hiding this comment.
Hi Gemini code assist.
That would iterate the codes twice, this only iterates it once.
(I'll admit the code is not built for efficiency, and the list is likely to be very short to begin with, but there is no need to do double work.)
1 participant
Makes
AnsiCode.toString()returnAnsiCode.escape, so the code can be embedded in strings directly like"a ${red}red${resetAll} word".(This is a potentially breaking change, if anyone relies on the exact
toStringoutput.The text does not appear to be designed to be used for anything but debugging.)
Makes
AnsiCodeTypeanenum._Potentially a breaking change if anyone is doing an non-exhaustive switch over
AnsiCodeType.This also changes the
toStringof the enum values fromAnsiCode.<name>toAnsiCodeType.<name>.(There was no reason for the original name.)
Makes the
wrapWithuse the reset codes for the Ansi style codes used, instead of always usingresetAll.If any color is included (or any custom
AnsiCodethat hasresetAllas reset code), it still uses a singleresetAll.May affect code that combines Ansi codes used directly with
wrapWith, if it expects a laterwrapWithto reset more than its own styles.