-
-
Notifications
You must be signed in to change notification settings - Fork 310
feat(config): add warning for multiple configuration files and update documentation #1773
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
Open
nicoleman0
wants to merge
10
commits into
commitizen-tools:master
Choose a base branch
from
nicoleman0:fix-config-warning
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+102
−2
Open
Changes from all commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
c03d500
feat(config): add warning for multiple configuration files
nicoleman0 ad49e81
Merge branch 'master' into fix-config-warning
nicoleman0 87bf26f
refactor: simplify multiple config files warning
nicoleman0 f825cd9
refactor(config): use walrus operator for git_project_root assignment
nicoleman0 77c2cdf
fix(config): only check for multiple configs when filepath is None
nicoleman0 6b8aef4
fix(config): improve warning message clarity for multiple config files
nicoleman0 f2c8e4f
refactor(config): remove _check_and_warn_multiple_configs function; r…
nicoleman0 f0fc425
fix(config): streamline multiple config files warning logic
nicoleman0 e900199
Merge branch 'master' into fix-config-warning
nicoleman0 9afa4f9
Merge branch 'master' into fix-config-warning
nicoleman0 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -239,6 +239,90 @@ def test_load_empty_pyproject_toml_from_config_argument(_, tmpdir): | |
| with pytest.raises(ConfigFileIsEmpty): | ||
| config.read_cfg(filepath="./not_in_root/pyproject.toml") | ||
|
|
||
| def test_warn_multiple_config_files(_, tmpdir, capsys): | ||
| """Test that a warning is issued when multiple config files exist.""" | ||
| with tmpdir.as_cwd(): | ||
| # Create multiple config files | ||
| tmpdir.join(".cz.toml").write(PYPROJECT) | ||
| tmpdir.join(".cz.json").write(JSON_STR) | ||
|
|
||
| # Read config | ||
| cfg = config.read_cfg() | ||
|
|
||
| # Check that the warning was issued | ||
| captured = capsys.readouterr() | ||
| assert "Multiple config files detected" in captured.err | ||
| assert ".cz.toml" in captured.err | ||
| assert ".cz.json" in captured.err | ||
| assert "Using" in captured.err | ||
|
|
||
| # Verify the correct config is loaded (first in priority order) | ||
| assert cfg.settings == _settings | ||
|
|
||
| def test_warn_multiple_config_files_with_pyproject(_, tmpdir, capsys): | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But technically, it's multiple configs. We probably should handle it better 🤔 |
||
| """Test warning excludes pyproject.toml from the warning message.""" | ||
| with tmpdir.as_cwd(): | ||
| # Create multiple config files including pyproject.toml | ||
| tmpdir.join("pyproject.toml").write(PYPROJECT) | ||
| tmpdir.join(".cz.json").write(JSON_STR) | ||
|
|
||
| # Read config - should use pyproject.toml (first in priority) | ||
| cfg = config.read_cfg() | ||
|
|
||
| # No warning should be issued as only one non-pyproject config exists | ||
| captured = capsys.readouterr() | ||
| assert "Multiple config files detected" not in captured.err | ||
|
|
||
| # Verify the correct config is loaded | ||
| assert cfg.settings == _settings | ||
|
|
||
| def test_warn_multiple_config_files_uses_correct_one(_, tmpdir, capsys): | ||
| """Test that the correct config file is used when multiple exist.""" | ||
| with tmpdir.as_cwd(): | ||
| # Create .cz.json with different settings | ||
| json_different = """ | ||
| { | ||
| "commitizen": { | ||
| "name": "cz_conventional_commits", | ||
| "version": "2.0.0" | ||
| } | ||
| } | ||
| """ | ||
| tmpdir.join(".cz.json").write(json_different) | ||
| tmpdir.join(".cz.toml").write(PYPROJECT) | ||
|
|
||
| # Read config - should use pyproject.toml (first in defaults.CONFIG_FILES) | ||
| # But since pyproject.toml doesn't exist, .cz.toml is second in priority | ||
| cfg = config.read_cfg() | ||
|
|
||
| # Check that warning mentions both files | ||
| captured = capsys.readouterr() | ||
| assert "Multiple config files detected" in captured.err | ||
| assert ".cz.toml" in captured.err | ||
| assert ".cz.json" in captured.err | ||
|
|
||
| # Verify .cz.toml was used (second in priority after pyproject.toml) | ||
| assert cfg.settings["name"] == "cz_jira" # from PYPROJECT | ||
| assert cfg.settings["version"] == "1.0.0" | ||
|
|
||
| def test_no_warn_with_explicit_config_path(_, tmpdir, capsys): | ||
| """Test that no warning is issued when user explicitly specifies config.""" | ||
| with tmpdir.as_cwd(): | ||
| # Create multiple config files | ||
| tmpdir.join(".cz.toml").write(PYPROJECT) | ||
| tmpdir.join(".cz.json").write(JSON_STR) | ||
|
|
||
| # Read config with explicit path | ||
| cfg = config.read_cfg(filepath=".cz.json") | ||
|
|
||
| # No warning should be issued | ||
| captured = capsys.readouterr() | ||
| assert "Multiple config files detected" not in captured.err | ||
|
|
||
| # Verify the explicitly specified config is loaded (compare to expected JSON config) | ||
| json_cfg_expected = JsonConfig(data=JSON_STR, path=Path(".cz.json")) | ||
| assert cfg.settings == json_cfg_expected.settings | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( | ||
| "config_file, exception_string", | ||
|
|
||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This for loop looks strange... I think we should remove the for loop
but that can be another PR
iirc the underlying logic of
BaseConfigis a bit confusingUh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, so should the code just use
config_candidates[0]directly with the first file found, whether it's empty or not?If so, the behavior would change to:
ConfigFileIsEmptyif empty (same as now)BaseConfig()if it's empty (currently skips empty files)This would be simpler and align the warning message with actual behavior. The only downside is if someone has an empty .cz.toml and a valid .cz.json, it would now ignore the .cz.json (whereas currently it would use it).
Shall I go ahead with this simplification in this PR, or would you prefer a separate one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could just add a TODO to remind the future contributors to address this issue. The main goal for this PR is just to add a warning for multiple configuration.