Skip to content

Conversation

XiaoliChan
Copy link
Contributor

@XiaoliChan XiaoliChan commented Oct 23, 2023

  • use os.isatty(sys.stdout.fileno()) is much better than sys.stdout.isatty()

Fixed:

@hugovk
Copy link
Member

hugovk commented Oct 23, 2023

Thanks for the PR, please could you explain how this is better?

@hugovk hugovk added the changelog: Changed For changes in existing functionality label Oct 23, 2023
@XiaoliChan
Copy link
Contributor Author

Thanks for the PR, please could you explain how this is better?

The os.isatty(sys.stdout.fileno()) is do more low-level check than sys.stdout.isatty()

@hugovk
Copy link
Member

hugovk commented Oct 23, 2023

Why is the CI failing with io.UnsupportedOperation: fileno?

@XiaoliChan
Copy link
Contributor Author

Why is the CI failing with io.UnsupportedOperation: fileno?

Weird

@XiaoliChan
Copy link
Contributor Author

Let me do more test

@XiaoliChan XiaoliChan force-pushed the better-color-check branch 2 times, most recently from cd77b9d to 7ecd513 Compare October 23, 2023 11:56
@codecov
Copy link

codecov bot commented Oct 23, 2023

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (47b5c25) 100.00% compared to head (df006ef) 97.90%.

Additional details and impacted files
@@             Coverage Diff             @@
##              main      #56      +/-   ##
===========================================
- Coverage   100.00%   97.90%   -2.10%     
===========================================
  Files            4        4              
  Lines          125      143      +18     
===========================================
+ Hits           125      140      +15     
- Misses           0        3       +3     
Flag Coverage Δ
macos-latest 97.90% <85.00%> (-2.10%) ⬇️
ubuntu-latest 97.90% <85.00%> (-2.10%) ⬇️
windows-latest 97.90% <85.00%> (-2.10%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
tests/test_termcolor.py 100.00% <100.00%> (ø)
src/termcolor/termcolor.py 94.33% <66.66%> (-5.67%) ⬇️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@XiaoliChan
Copy link
Contributor Author

@hugovk Just update the test_termcolor.py to make it pass the check

Signed-off-by: Xiaoli Chan <2209553467@qq.com>
@hugovk
Copy link
Member

hugovk commented Oct 25, 2023

Thanks for the PR, please could you explain how this is better?

The os.isatty(sys.stdout.fileno()) is do more low-level check than sys.stdout.isatty()

But why is this better?


Please could you fix the mypy error?

Also it looks like some other people also check for io.UnsupportedOperation - should we do that too?

https://github.com/pdm-project/pdm/blob/99e7877c98bf9beaa8468b1e6ee3365778a63c72/install-pdm.py#L167-L170

@XiaoliChan
Copy link
Contributor Author

XiaoliChan commented Oct 26, 2023

But why is this better?

I can't explain this sorry, but it actually works better than sys.stdout.isatty(), so I think it do more deep low-level check.

@NeffIsBack
Copy link

NeffIsBack commented Oct 26, 2023

We currently have the problem that when displaying coloured output through rich.console the isatty() check fails.
Pennyw0rth/NetExec#86

Other popular projects like sqlmap uses fileno() as check if a terminal is attached https://github.com/sqlmapproject/sqlmap/blob/7a6abb56d29225b73d333df00ce06012517c5553/lib/core/settings.py#L262 and this method works for rich.console as well, so this might be a better check, although I am not super familiar with the low level checks that are done.

pre-commit-ci bot and others added 3 commits October 26, 2023 17:30
Signed-off-by: Xiaoli Chan <2209553467@qq.com>
Signed-off-by: Xiaoli Chan <2209553467@qq.com>
XiaoliChan and others added 3 commits November 1, 2023 12:17
Co-authored-by: Hugo van Kemenade <hugovk@users.noreply.github.com>
Signed-off-by: Xiaoli Chan <2209553467@qq.com>
@XiaoliChan
Copy link
Contributor Author

Why the workflow stucked here XD

@hugovk
Copy link
Member

hugovk commented Nov 7, 2023

Why the workflow stucked here XD

I need to approve it for first-time contributors:

image

@XiaoliChan
Copy link
Contributor Author

I need to approve it for first-time contributors:

XD

So I think it can be merged, how do you think of it?

@hugovk hugovk merged commit 3fec189 into termcolor:main Nov 12, 2023
@hugovk
Copy link
Member

hugovk commented Nov 12, 2023

Thank you!

@XiaoliChan XiaoliChan deleted the better-color-check branch November 14, 2023 16:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog: Changed For changes in existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants