Fix 1216 explicitly deprecate setuptools dynamic version when active #1219
Conversation
RonnyPfannschmidt
force-pushed
the
fix-1216-explicitly-deprecate-setuptools-dynamic-version-when-active
branch
from
ff20cbb to
629f4f4
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR adds explicit warnings for misconfigurations where users have incorrectly combined setuptools-scm with setuptools dynamic version configurations, a common cargo-culted antipattern that sabotages setuptools-scm functionality.
- Detects and warns when
version = attr:is used in setup.cfg metadata - Detects and warns when
[tool.setuptools.dynamic]version configuration is present in pyproject.toml - Nullifies the problematic version configurations to prevent interference with setuptools-scm
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/setuptools_scm/_integration/setup_cfg.py | Adds warning detection for attr-based version in setup.cfg and nullifies it |
| src/setuptools_scm/_integration/pyproject_reading.py | Adds warning detection for setuptools dynamic version in pyproject.toml and supports test injection |
| testing/test_integration.py | Adds test coverage for setup.cfg dynamic version warning behavior |
| testing/test_pyproject_reading.py | Adds test coverage for pyproject.toml dynamic version warning and test utilities |
| src/setuptools_scm/_config.py | Minor whitespace cleanup |
| CHANGELOG.md | Documents the fix for issue #1216 |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| tool_name: str = DEFAULT_TOOL_NAME, | ||
| canonical_build_package_name: str = "setuptools-scm", | ||
| _given_result: _t.GivenPyProjectResult = None, | ||
| _given_definition: TOML_RESULT | None = None, |
There was a problem hiding this comment.
The _given_definition parameter appears to be a testing-only feature but lacks documentation in the docstring. Consider adding documentation for this parameter or making it clear that it's for internal testing use only.
Copilot uses AI. Check for mistakes.
|
|
||
|
|
||
| ## v9.2.0 | ||
|
|
There was a problem hiding this comment.
There are two ## v9.2.0 sections in the changelog, which will create confusion. The second one should likely be a different version number or these sections should be merged.
Copilot uses AI. Check for mistakes.
| raise _given_result | ||
|
|
||
| defn = read_toml_content(path) | ||
| if _given_definition is not None: |
There was a problem hiding this comment.
[nitpick] The conditional logic for _given_definition vs read_toml_content(path) should be moved earlier in the function, before the existing _given_result handling, to maintain consistent parameter precedence and avoid potential confusion about which takes priority.
Copilot uses AI. Check for mistakes.
| warnings.warn( | ||
| f"{path}: at [{section}]\n" | ||
| f"{expression} forcing setuptools to override the version setuptools-scm sets\n" | ||
| "When using setuptools-scm its invalid to use setuptools dynamic version as well, please removeit.\n" |
There was a problem hiding this comment.
Missing space in removeit.
Edit: its => it is (or it's)
| def warn_dynamic_version(path: Path, section: str, expression: str) -> None: | ||
| warnings.warn( | ||
| f"{path}: at [{section}]\n" | ||
| f"{expression} forcing setuptools to override the version setuptools-scm sets\n" |
There was a problem hiding this comment.
English isn't my first language, but I think there's a missing "is" before "forcing", and possibly a missing "that" before "setuptools-scm".
closes #1216
as enough users cargo culted layering dynamic version from version file into setuptools_scm usage, we explicitly warn about it + its use in setup.cfg/pyproject.toml