Skip to content

Conversation

sbc100
Copy link
Collaborator

@sbc100 sbc100 commented Oct 6, 2025

Followup to #25502.

@sbc100 sbc100 requested review from juj and kripken October 6, 2025 21:30
@juj
Copy link
Collaborator

juj commented Oct 6, 2025

It would be cool to land #25495 first.. now this is going again into the "refactor-everything-first-to-create-merge-conflicts" territory.

Copy link
Collaborator

@juj juj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Do you have a Windows system to test that this works on Windows?

@sbc100
Copy link
Collaborator Author

sbc100 commented Oct 6, 2025

LGTM. Do you have a Windows system to test that this works on Windows?

I don't have a windows machine myself but we do run test_color_diagnostics_force on all platforms (its marked as @crossplatform). The test conforms that the ANSI codes are printed correctly the stderr on all platforms.

Whether the platform actually renders those codes correctly we do not (cannot reasonably really) test.

@sbc100 sbc100 force-pushed the ansi_color_followup branch from 4e40cae to 6811349 Compare October 6, 2025 22:57
@sbc100 sbc100 changed the title Cleanup diagnostics.diag function. NFC =Unify handling of console color output. NFC Oct 6, 2025
@sbc100 sbc100 changed the title =Unify handling of console color output. NFC Unify handling of console color output. NFC Oct 6, 2025
@sbc100
Copy link
Collaborator Author

sbc100 commented Oct 6, 2025

Happy to see #25495 land first.

I increased the scope of this change to unify the color handling between the logger and diagnostics code.

@sbc100 sbc100 force-pushed the ansi_color_followup branch 2 times, most recently from 8f11b47 to ecf9b49 Compare October 7, 2025 00:05
@sbc100 sbc100 requested a review from juj October 7, 2025 00:06
@sbc100 sbc100 enabled auto-merge (squash) October 7, 2025 00:06
Copy link
Collaborator

@juj juj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

@sbc100 sbc100 force-pushed the ansi_color_followup branch from ecf9b49 to 4f6e36b Compare October 7, 2025 00:34
@sbc100 sbc100 disabled auto-merge October 7, 2025 01:18
@sbc100 sbc100 merged commit 7f69e69 into emscripten-core:main Oct 7, 2025
24 of 33 checks passed
@sbc100 sbc100 deleted the ansi_color_followup branch October 7, 2025 01:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants