Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove ANSI colors when writing to external warnings file. #11624

Merged

Conversation

picnixz
Copy link
Member

@picnixz picnixz commented Aug 20, 2023

Fix #11617.
Fix #11574.

In addition, I needed to fix #11574 so that testing can be done properly (otherwise the file descriptor is kept opened in write mode and it is not possible to correctly read it).

@AA-Turner Can you bump the development version 7.2.3 (or 7.3) so that I can add a CHANGES entry please?

@mgeier
Copy link
Contributor

mgeier commented Aug 26, 2023

Is the --keep-colors option really necessary?

I can't imagine anyone wanting those escape sequences in their warnings file.

More importantly, I think adding this option would cause confusion with the already existing --color and --no-color options.

@picnixz
Copy link
Member Author

picnixz commented Aug 27, 2023

Err.. I don't really know. Like sometimes you may want to keep the colors when you print them using some weird commands. Or for filtering.

It was just to ensure that if there was someone out there doing it, then they don't need to change much (although I would recommend using tee instead then).

@picnixz
Copy link
Member Author

picnixz commented Aug 28, 2023

On a second thought, I think not having that option would be the best. People that want to keep colors should simply tee the standard output I think.

Since I am away, I won't be able to update the PR soon. At least, it would simplify a bit the logic and reduce the number of CLI options for the end-user to remember.

@picnixz
Copy link
Member Author

picnixz commented Sep 20, 2023

So, I removed --keep-colors since it's indeed not a good idea to keep (and it simplified the flow).

sphinx/cmd/build.py Outdated Show resolved Hide resolved
sphinx/cmd/build.py Outdated Show resolved Hide resolved
@picnixz
Copy link
Member Author

picnixz commented Sep 21, 2023

I'll make the changes today or tomorrow and add a CHANGES entry as well.

CHANGES.rst Outdated Show resolved Hide resolved
Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>
@AA-Turner AA-Turner merged commit 5fe0bd4 into sphinx-doc:master Sep 21, 2023
22 of 23 checks passed
@AA-Turner
Copy link
Member

Thanks @picnixz!

@picnixz picnixz deleted the feature/11617-no-ANSI-in-warnings-file branch September 21, 2023 09:04
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants