Skip to content

Conversation

@firewave
Copy link
Collaborator

@firewave firewave commented Mar 2, 2023

The colors and special characters are fixed replacements which will be identical for each message so we can simply replace those once instead of doing it for each message.

We cannot re-use the new logic from replace() for the other fields as the generation of the map is slower than the repeated scanning of the string.

@firewave firewave marked this pull request as draft March 2, 2023 12:41
@firewave firewave marked this pull request as ready for review March 2, 2023 13:54
@firewave firewave force-pushed the template branch 2 times, most recently from fb83649 to e9693db Compare March 3, 2023 12:45
@firewave
Copy link
Collaborator Author

firewave commented Mar 9, 2023

Anything still to do here?

@firewave
Copy link
Collaborator Author

If #4882 is merged first we don't need the explicit color handling in tests.

@firewave firewave marked this pull request as ready for review April 8, 2023 16:46
@firewave
Copy link
Collaborator Author

firewave commented Apr 8, 2023

I integrated the change from #4941.

@firewave
Copy link
Collaborator Author

firewave commented Apr 8, 2023

The stream checking logic is not working as intended because the templates are strings and the colors are resolved before the stream is being called. This will lead to non-colored messages on stderr if you only redirect stdout and vice versa. The behavior is still slightly improved though.

I have no idea how to address this yet and we are also not checking the color conversion anywhere. The testing can probably needs to be done with some stream redirection and I will not tackle this in this PR.

@danmar danmar merged commit cfca3a6 into danmar:main Apr 8, 2023
@danmar
Copy link
Owner

danmar commented Apr 8, 2023

This will lead to non-colored messages on stderr if you only redirect stdout and vice versa

At least that is better than colored messages to stderr if you don't redirect stdout.

@firewave firewave deleted the template branch April 13, 2023 10:46
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