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

Fixes issue #2606 by making color=True behavior agnostic of Operating System. #2731

Open
wants to merge 1 commit into
base: stable
Choose a base branch
from

Conversation

Tyl13
Copy link
Contributor

@Tyl13 Tyl13 commented May 21, 2024

Issue #2606 described a inconsistency in how color=True works between Operating Systems.
This is because ANSI styles can't always be applied to different terminals on windows. This did not cause issues when running it within the terminals because Colorama would change the ANSI style codes to escape codes for windows when it was required. However, if you were to pipe the click.echo() output to a file then the should_strip_ansi function would not work on Windows as the other operating systems.

This is because the auto_wrap_for_ansi function was not being called with the color argument. After the auto_wrap_for_ansi function is called, it defaults color=None and will end up calling should_strip_ansi with color=None which is different from the original call to should_strip_ansi that was called before the wrapper function.

By passing color to auto_wrap_for_ansi, then the behavior of should_strip_ansi becomes consistent.

An additional change applied within the PR is to the tests in test_utils.py. This is to increase test coverage for the different cases of click.echo() and removes the old version of the test. Instead of one test that mocks isatty(), there are now three different tests. One that mocks isatty()=True and _is_jupyter_kernel_output()=False, another that mocks isatty()=False and _is_jupyter_kernel_output()=True , and the last one mocks both isatty()=False and _is_jupyter_kernel_output()=False.

fixes #2606

  • Add tests that demonstrate the correct behavior of the change. Tests
    should fail without the change.
  • Add or update relevant docs, in the docs folder and in code.
  • Add an entry in CHANGES.rst summarizing the change and linking to the issue.

@davidism davidism added this to the 8.1.8 milestone May 21, 2024
@Tyl13
Copy link
Contributor Author

Tyl13 commented May 22, 2024

Rebased to fix merge conflicts more easily. However, something seems to sometimes cause the windows version to fail. Currently investigating that. I've had it fail locally and then a second rerun succeed. I'm unsure what is causing the issue atm.

@Tyl13
Copy link
Contributor Author

Tyl13 commented May 23, 2024

The issue is coming from my previous change to the runner. The issue is documented here #2732 with the PR to fix it here #2733.

@AndreasBackx
Copy link
Collaborator

AndreasBackx commented Nov 9, 2024

Thank you, though this was recently fixed by #2607.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 24, 2024
@davidism
Copy link
Member

davidism commented Dec 4, 2024

Looks like we may have concluded that as well, and now this PR was only test changes? I can't figure out why color was defined as an argument but was never being passed to that function, tried looking through blame history but couldn't figure it out. I'm going to reopen this and see if the tests still merge, as they look like an improvement at first glance.

@davidism davidism reopened this Dec 4, 2024
@pallets pallets unlocked this conversation Dec 4, 2024
@davidism davidism removed this from the 8.1.8 milestone Dec 4, 2024
@davidism
Copy link
Member

davidism commented Dec 4, 2024

After rebasing this on latest stable that includes #2607 and #2733, the test_echo_color_flag that you change fails now. I have a feeling the test doesn't need to be changed if the code was changed. I'm not going to block 8.1.8 on this since it's only a change to tests. @Tyl13 if you still remember any discussion about this from the sprint, let me know, and if you can update the test to pass again that would be great.

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.

3 participants