Repository navigation
fix: preserve distinct default configuration identities - #430
codeforester merged 5 commits into
Conversation
codeforester
left a comment
There was a problem hiding this comment.
Reviewed head f17e78c against #425. The dotted-identity bug is real and fixed: acme.tools previously collapsed to acme, and now gets its own directory. Explicit user_config_dir stays authoritative. Two blocking issues.
1. Every capitalized application name silently loses its existing user config. config_namespace_component reuses runtime_namespace_component, which lowercases names and appends a digest whenever the readable slug differs from the input. I compared the default directory on main and on this branch:
cli_name |
main | this PR |
|---|---|---|
basectl, mytool, gh-dash, my_tool |
unchanged | unchanged |
acme.tools |
acme |
acme.tools ✅ (the #425 fix) |
MyTool |
MyTool |
mytool--0e6f80c36435 |
Base |
Base |
base--7b47361aad19 |
Alpha-Tool (already normalized) |
Alpha-Tool |
alpha-tool--dd7ba9aae023 |
AWSCLI |
AWSCLI |
awscli--597bec206248 |
After upgrading, an app named MyTool stops reading ~/.config/MyTool/config.yaml, with no warning. The docs say old directories aren't migrated, which satisfies #425's "do not silently move", but the user-visible result is silent config loss. #425 also asks for coverage of "ordinary existing names", and the only ordinary name tested is lowercase beta. Options:
- Keep the old directory when it's collision-free, and add a digest only for real collision cases (dots that used to be truncated,
Alpha ToolvsAlpha-Tool). - Or, when the legacy
default_config_root() / normalize_cli_name(cli_name)exists and the new one doesn't, keep using the legacy directory or at least log a one-time warning naming both paths.
Either way, please add MyTool/Alpha-Tool to the tests and say which behavior is intended.
2. CI: Quality and security gates fails with ruff format: "File would be reformatted --> lib/python/base_cli/profile.py:235:41".
Minor: config_namespace_component is a pure alias of runtime_namespace_component. That's fine if it's meant as a seam for the config policy to diverge later (option 1 above would use exactly that), but say so in the docstring.
c640617 to
972b635
Compare
f17e78c to
4862c5c
Compare
|
Follow-up on the review findings:
Validation: 34 focused tests, strict mypy, Ruff check, Ruff format, and diff checks pass locally. The PR head is now |
|
Re-verified at |
Summary
Use the collision-resistant application identity namespace for default user
configuration directories in both batteries-included entry points. Dotted
identities remain distinct, names that would collide after normalization receive
stable identity-specific components, and explicit
user_config_dirvalues remainauthoritative.
The documentation records the path contract and explains that older normalized
directories are not moved, merged, or deleted automatically.
Issue
Fixes #425
Validation
UV_CACHE_DIR=/private/tmp/base-cli-uv-cache uv run --extra dev --extra typer pytest -q(661 passed, 2 skipped)UV_CACHE_DIR=/private/tmp/base-cli-uv-cache uv run --extra quality mypy --strict lib/python/base_cliUV_CACHE_DIR=/private/tmp/base-cli-uv-cache uv run --extra quality ruff check lib/python/base_cli/paths.py lib/python/base_cli/config.py lib/python/base_cli/profile.py tests/test_paths.py tests/test_batteries_included_config.pygit diff --checkTrain
This car follows the v0.5.0 validation car for #424. It is based on that
branch so the two implementation changes can be reviewed and merged in order.