Skip to content

Conversation

@Arcitec
Copy link
Contributor

@Arcitec Arcitec commented Aug 2, 2025

Spotted this issue while looking at some recent changes.

Fixes ConfigManager to always use the defined constants, so that it won't break if the constant values are changed in the future.

I also ripgrep'd the rest of the codebase and did not come up with any other locations where the strings are used instead of the constants, so it seems fine now.


I was unsure about this location though:

class WorkspaceType(Enum):
CURRENT_DIR = "current_dir"
DEFAULT = "default"
SPECIFIED = "specified"
RECENT = "recent"

It doesn't seem like it should be using CONTEXT_KEY_RECENT = "recent" instead of hardcoding. Perhaps someone can confirm that the workspace manager keys are indeed totally separate from the config keys.

@dosubot dosubot bot added the size:S This PR changes 10-29 lines, ignoring generated files. label Aug 2, 2025
@codecov
Copy link

codecov bot commented Aug 2, 2025

Codecov Report

❌ Patch coverage is 20.00000% with 4 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
comfy_cli/config_manager.py 20.00% 4 Missing ⚠️
@@           Coverage Diff           @@
##             main     #299   +/-   ##
=======================================
  Coverage   47.96%   47.96%           
=======================================
  Files          30       30           
  Lines        3077     3077           
=======================================
  Hits         1476     1476           
  Misses       1601     1601           
Files with missing lines Coverage Δ
comfy_cli/config_manager.py 50.00% <20.00%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@dosubot dosubot bot added the bug Something isn't working label Aug 2, 2025
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.

Perhaps someone can confirm that the workspace manager keys are indeed totally separate from the config keys.

I guess WorkspaceType is related only to path of workspace.

It was added by this commit

so no need to replace recent there with CONTEXT_KEY_RECENT (at least for now)


Regarding this PR - I think we can replace the values with constants, logically everything seems fine.

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Aug 3, 2025
@bigcat88 bigcat88 merged commit 0a4aa3e into Comfy-Org:main Aug 3, 2025
10 of 11 checks passed
@Arcitec Arcitec deleted the fix-configmanager-constants branch August 3, 2025 16:46
@Arcitec
Copy link
Contributor Author

Arcitec commented Aug 3, 2025

@bigcat88 Ah yeah from the commit you linked, it seems like the WorkspaceType enum is just a marker to tell the caller "what type of workspace path was returned", so that part shouldn't be changed. Good good. :) 👍

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

Labels

bug Something isn't working lgtm This PR has been approved by a maintainer size:S This PR changes 10-29 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants