Skip to content

Conversation

@jtydhr88
Copy link
Collaborator

bounty task https://www.notion.so/drip-art/Add-Comfy-CLI-feature-to-check-out-a-single-PR-launch-comfy-and-run-a-set-of-tests-1f36d73d36508074b1c3fc56d13f4346?source=copy_link

add --pr arg, support format:

  • "username:branch-name"
  • PR number such as "#123"
  • PR link, such as "https://github.com/comfyanonymous/ComfyUI/pull/123"

full command:
comfy --workspace=G:\comfy-cli-test install --pr "https://github.com/comfyanonymous/ComfyUI/pull/8597"
then
comfy --workspace=G:\comfy-cli-test launch

@dosubot dosubot bot added the size:L This PR changes 100-499 lines, ignoring generated files. label Jun 22, 2025
@codecov
Copy link

codecov bot commented Jun 22, 2025

Codecov Report

Attention: Patch coverage is 80.12422% with 32 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
comfy_cli/command/install.py 73.07% 28 Missing ⚠️
comfy_cli/cmdline.py 33.33% 2 Missing ⚠️
comfy_cli/git_utils.py 95.12% 2 Missing ⚠️
Flag Coverage Δ
unittests 48.14% <80.12%> (+2.56%) ⬆️

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

Files with missing lines Coverage Δ
comfy_cli/command/github/pr_info.py 100.00% <100.00%> (ø)
comfy_cli/cmdline.py 47.67% <33.33%> (-0.17%) ⬇️
comfy_cli/git_utils.py 67.64% <95.12%> (+41.72%) ⬆️
comfy_cli/command/install.py 47.81% <73.07%> (+14.96%) ⬆️

... and 5 files with indirect coverage changes

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

@dosubot dosubot bot added the enhancement New feature or request label Jun 22, 2025
@dosubot dosubot bot added size:XL This PR changes 500-999 lines, ignoring generated files. and removed size:L This PR changes 100-499 lines, ignoring generated files. labels Jun 22, 2025
Copy link
Contributor

@christian-byrne christian-byrne left a comment

Choose a reason for hiding this comment

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

Strengths

  • Excellent test coverage (376 lines of tests)
  • Clean separation of concerns
  • Good error handling with rich formatting
  • Handles both fork and same-repo PRs correctly

Possible Improvements

Critical:

  1. Parameter validation bug (cmdline.py:348): The condition version != "nightly" or commit will be true if version is None, preventing valid use cases. Should be version not in {None, "nightly"} or commit.

Important:

  1. Timestamp formatting (install.py:381): The reset_time is displayed as a Unix timestamp instead of human-readable format.
  2. Deleted fork handling (install.py:579): Missing null check will cause KeyError if PR is from a deleted fork.
  3. Branch name sanitization (git_utils.py:87): Git will reject branch names with special characters.

Suggestions:

  1. Docstring placement (install.py:184): Move docstring before code execution.
  2. Remote naming (git_utils.py:70): Consider using pr-{number}-{user} to avoid conflicts when checking out multiple PRs from same author.

@jtydhr88
Copy link
Collaborator Author

jtydhr88 commented Jun 24, 2025

@christian-byrne fixed your feedback except Timestamp formatting, this is an existing code used in checkout version, I just move it into a function could be shared in my new logic

@jtydhr88 jtydhr88 requested a review from christian-byrne June 24, 2025 21:17
@jtydhr88
Copy link
Collaborator Author

btw, regarding to failing test, I checked it and likely failed on download something on the MAC machine, not caused by my changes I believe

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Jun 25, 2025
@jtydhr88 jtydhr88 merged commit 8ee6f42 into Comfy-Org:main Jun 25, 2025
11 checks passed
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:XL This PR changes 500-999 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants