Convert setup.py to pyproject.toml
#593
Conversation
|
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
mariocj89
force-pushed
the
pu/modernize-packaging
branch
2 times, most recently
from
f5db213 to
9fc12fd
Compare
Move to declarative packaging to provide better tooling integration and build isolation (by enabling `python-build`). This will run the build in isolation with the environment and being explicit about the dependencies that are used to build the sdists and bdists, making sure that the build is reproducible and isolated. The check for the version compatibility has been moved to a unit test, to move any scripting out of the project metadata.
mariocj89
force-pushed
the
pu/modernize-packaging
branch
from
9fc12fd to
98188bd
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| classifiers = [ | ||
| "Development Status :: 5 - Production/Stable", | ||
| "Operating System :: Microsoft :: Windows", | ||
| "Operating System :: MacOS :: MacOS X", | ||
| "Operating System :: POSIX :: Linux", | ||
| "Programming Language :: Python :: 3.9", | ||
| "Programming Language :: Python :: 3.10", | ||
| "Programming Language :: Python :: 3.11", | ||
| "Programming Language :: Python :: 3.12", | ||
| ] |
There was a problem hiding this comment.
The license classifier is missing from the classifiers list. The original setup.py included 'License :: OSI Approved :: Apache Software License'. This classifier should be added to maintain consistency with the previous configuration and properly declare the license in package metadata.
| clickhouse_version_str = __version__.version | ||
|
|
||
| # Parse versions | ||
| dbt_version = Version(dbt_version_str) |
There was a problem hiding this comment.
Double space between 'assert' and 'clickhouse_version'.
| dbt_version = Version(dbt_version_str) | |
| dbt_version = Version(dbt_version_str) | |
| dbt_minor = f"{dbt_version.major}.{dbt_version.minor}" |
| "dbt-adapters>=1.16.7,<1.17", | ||
| "clickhouse-connect>=0.10.0", | ||
| "clickhouse-driver>=0.2.10", | ||
| "setuptools>=69", |
There was a problem hiding this comment.
There's an inconsistency in setuptools version requirements. The build-system section requires 'setuptools>=64', but the runtime dependencies specify 'setuptools>=69'. The original setup.py had 'setuptools>=0.69' which appears to be a typo. Consider aligning these requirements - typically the build-system requirement should match or be compatible with the runtime requirement. If setuptools>=69 is intended, the build-system should also require >=69. Note that setuptools 69 is a recent version and may limit compatibility.
| "setuptools>=69", | |
| "setuptools>=64", |
| dynamic = ["version"] | ||
| description = "The Clickhouse plugin for dbt (data build tool)" | ||
| readme = "README.md" | ||
| license = "MIT" |
There was a problem hiding this comment.
The license specified as "MIT" contradicts the actual LICENSE file in the repository, which contains the Apache License Version 2.0. The original setup.py correctly used 'License :: OSI Approved :: Apache Software License' in its classifiers. This should be changed to either license = "Apache-2.0" or the correct SPDX identifier for the Apache License.
| license = "MIT" | |
| license = "Apache-2.0" |
| # The dbt-clickhouse version should be smaller than or equal to the installed dbt version | ||
| assert clickhouse_version <= dbt_version, ( | ||
| f"Invalid metadata in pyproject.toml: package_version={clickhouse_version_str} must start with " | ||
| f"dbt_version={dbt_minor}. The adapter version should indicate the minimum supported " |
There was a problem hiding this comment.
Undefined variable dbt_minor in assertion error message
Medium Severity
The variable dbt_minor is used in the assertion error message but is never defined in the test file. When the version compatibility check fails and the assertion triggers, Python will raise a NameError instead of displaying the intended helpful error message. The original setup.py had dbt_minor_version = '1.9' which was used for this purpose, but it wasn't included in the new test implementation.
2 participants
Summary
Move to declarative packaging to provide better tooling integration and build isolation (by enabling
python-build).This will run the build in isolation with the environment and being explicit about the dependencies that are used to build the sdists and bdists, making sure that the build is reproducible and isolated.
The check for the version compatibility has been moved to a unit test, to move any scripting out of the project metadata.
Checklist
Delete items not relevant to your PR:
Let me know if you want me to add things to changelog/docs.
Relates to #592
Note
Migrates to declarative packaging and modern build tooling.
setup.pywithpyproject.toml(PEP 517), moving project metadata, dependencies, package data, and using dynamic version fromdbt.adapters.clickhouse.__version__.versionbuild/twineand build viapython -m buildinstead ofsetup.pytests/unit/test_version_compatibility.pyto assert the adapter version is not newer than the installeddbt-coreWritten by Cursor Bugbot for commit 98188bd. This will update automatically on new commits. Configure here.