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.)
natebosch
left a comment
natebosch
left a comment
There was a problem hiding this comment.
I think as long as we can land this without breaking outselves it makes sense to go forward with it.
If it does turn out to be too breaking we can instead use a major version bump.
| /// Standard ANSI escape code for customizing terminal text output. | ||
| /// | ||
| /// [Source](https://en.wikipedia.org/wiki/ANSI_escape_code#Colors) | ||
| class AnsiCode { |
There was a problem hiding this comment.
I wonder if we should make this base class now that we're relying on the existence of the _codes field. I don't think it was designed for extension outside the library, so we can maybe even make it final?
There was a problem hiding this comment.
Probably should. Maybe it's a major version-worthy change.
There was a problem hiding this comment.
In the past we have added class modifiers without major version bumps. I think it would probably be ok either way here. I don't see any classes that extend or implement this so we could go all the way to final if we want. I'd be happy to see it land as either base or final but I lean towards final unless we see a specific use case for extending it.
natebosch
left a comment
natebosch
left a comment
There was a problem hiding this comment.
The other changes look good. I think we should add base or final to AnsiCode and land this.
2 participants
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.