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

Add terminal size fallback #5623

Merged
merged 1 commit into from
Nov 19, 2024
Merged

Conversation

yoland68
Copy link
Collaborator

@yoland68 yoland68 commented Nov 15, 2024

os.get_terminal_size() can fail and causing logs to be unable to render in frontend

Copy link
Collaborator

@huchenlei huchenlei left a comment

Choose a reason for hiding this comment

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

Please specify context on why this change is needed.

@freakabcd
Copy link
Contributor

I'm a little bit confused by the diff. Why not just call shutil.get_terminal_size() within update_size()?

https://github.com/python/cpython/blob/main/Lib/shutil.py#L1438
seems to suggest that it handles all the cases and always returns a tuple (the fallback being (80,24)).

Also would highly suggest usage of black (or ruff; just the formatter not the checker) on a project wide basis.

@yoland68
Copy link
Collaborator Author

Please specify context on why this change is needed.

Added context

@huchenlei huchenlei added the Good PR This PR looks good to go, it needs comfy's final review. label Nov 18, 2024
@huchenlei
Copy link
Collaborator

@comfyanonymous Ping on review.

@comfyanonymous comfyanonymous merged commit f498d85 into master Nov 19, 2024
8 checks passed
@comfyanonymous comfyanonymous deleted the yo-add-get-term-size-fallback branch November 19, 2024 08:34
tiangles pushed a commit to diffus-me/ComfyUI that referenced this pull request Nov 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Good PR This PR looks good to go, it needs comfy's final review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants