Skip to content

Conversation

@neverbiasu
Copy link
Contributor

What changed

Updated the comfy env command to use our existing ui.display_table() function instead of manually creating Rich Table objects.

Why

  • Keeps table rendering consistent across the codebase
  • Follows the pattern we already use elsewhere
  • Makes the code cleaner by separating data from presentation

Changes made

  • Modified EnvChecker.fill_print_table() to return data tuples instead of creating a table
  • Updated WorkspaceManager.fill_print_table() to return data tuples too
  • Changed the env command to collect all data first, then render it with ui.display_table()

The output looks exactly the same to users.

after before
after before

@dosubot dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. enhancement New feature or request labels Jun 18, 2025
@robinjhuang robinjhuang requested a review from bigcat88 July 26, 2025 22:44
@robinjhuang
Copy link
Member

Seems like a reasonable change. @bigcat88 Can you take a look?

Copy link
Contributor

@bigcat88 bigcat88 left a comment

Choose a reason for hiding this comment

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

Overall the PR is good (tested locally), but some small changes need to be made.

@codecov
Copy link

codecov bot commented Jul 29, 2025

Codecov Report

❌ Patch coverage is 26.47059% with 25 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
comfy_cli/config_manager.py 33.33% 10 Missing ⚠️
comfy_cli/env_checker.py 0.00% 10 Missing ⚠️
comfy_cli/cmdline.py 42.85% 4 Missing ⚠️
comfy_cli/workspace_manager.py 50.00% 1 Missing ⚠️
Flag Coverage Δ
unittests 47.96% <26.47%> (+2.39%) ⬆️

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

Files with missing lines Coverage Δ
comfy_cli/workspace_manager.py 63.41% <50.00%> (+7.31%) ⬆️
comfy_cli/cmdline.py 47.67% <42.85%> (-0.17%) ⬇️
comfy_cli/config_manager.py 50.00% <33.33%> (-4.80%) ⬇️
comfy_cli/env_checker.py 46.80% <0.00%> (-2.13%) ⬇️

... and 12 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@bigcat88
Copy link
Contributor

thanks for the changes, can you remove the last unused import that ruff complains about so we can merge this?

@neverbiasu
Copy link
Contributor Author

sure, just removed it

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Aug 1, 2025
@bigcat88 bigcat88 merged commit 2575000 into Comfy-Org:main Aug 2, 2025
11 of 14 checks passed
@bigcat88
Copy link
Contributor

bigcat88 commented Aug 2, 2025

thanks for this contribution

@Arcitec
Copy link
Contributor

Arcitec commented Aug 2, 2025

Nice cleanup and unification. Good job! 👍

@neverbiasu
Copy link
Contributor Author

thx guys🤗

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request lgtm This PR has been approved by a maintainer size:M This PR changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants