build: pyproject migration, shared tooling (ruff/pylint/uv), lint cleanup & typing - #21
build: pyproject migration, shared tooling (ruff/pylint/uv), lint cleanup & typing#21vidiecan wants to merge 7 commits into
Conversation
Replace setup.py with a PEP 621 pyproject.toml (static metadata, requests core dep, solr + dev optional-dependency groups) and mirror the consumer repo's toolchain so both share one standard: - .pre-commit-config.yaml (mixed-line-ending, autopep8, ruff, pylint) - [tool.ruff] / [tool.pylint] / [tool.autopep8] / [tool.mypy] config - ship a py.typed marker (PEP 561) via package-data + MANIFEST.in Drop requirements.txt (fpdf/textblob/setuptools were unused cruft; runtime deps are now declared in pyproject). requirements-test.txt is kept because the consumer repo's CI installs it.
Add a ruff+pylint lint job and a mypy typecheck job (continue-on-error, since strict typing is being introduced incrementally) alongside the existing pytest matrix, all run via uvx. Key the pip cache off pyproject.toml now that setup.py is gone.
Bring dspace_rest_client to a clean 10.00/10 under the shared pylint config without changing behavior (all 68 tests still pass): - replace wildcard imports with explicit imports + __all__ (client, __init__) - extract a _is_valid_uuid() helper; stop shadowing the builtin id - fix a dangerous mutable default arg (proxies), list()/dict() literals, no-else-return, consider-using-in, inconsistent-return-statements, logging-not-lazy, pointless-string-statement and unused variables - collapse a redundant WorkspaceItem override to plain inheritance - add type hints (from __future__ import annotations) across models/client
Job-level continue-on-error still surfaces the job as a failed check on the PR; moving it to the mypy step lets the job pass green while mypy stays advisory.
There was a problem hiding this comment.
Pull request overview
This PR modernizes the dspace_rest_client library by migrating packaging/configuration to pyproject.toml, aligning local tooling with shared repo standards (pre-commit, ruff, pylint, mypy), and updating CI to run lint + typecheck jobs alongside the existing pytest matrix. It also includes largely behavior-preserving lint cleanups and type-hint additions across the client/models.
Changes:
- Migrates packaging from
setup.py/requirements.txtto PEP 621pyproject.toml, addsMANIFEST.in+py.typedmarker. - Adds shared developer tooling (
.pre-commit-config.yaml, ruff/pylint/mypy config inpyproject.toml) and CI lint/typecheck jobs usinguvx. - Performs lint-driven refactors and typing improvements in
dspace_rest_client/client.py,dspace_rest_client/models.py, and package exports indspace_rest_client/__init__.py.
Reviewed changes
Copilot reviewed 9 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| setup.py | Removed legacy setuptools build entrypoint in favor of pyproject.toml. |
| requirements.txt | Removed unused/legacy dependency list now represented in pyproject.toml. |
| requirements-test.txt | Updates CI/install commentary to reflect pyproject-based installs. |
| pyproject.toml | Adds PEP 621 metadata plus ruff/pylint/mypy/autopep8 configuration. |
| MANIFEST.in | Ensures README.md and py.typed are included in source distributions. |
| dspace_rest_client/py.typed | Marks the package as typed (PEP 561). |
| dspace_rest_client/models.py | Adds annotations and lint cleanups; simplifies redundant subclass override. |
| dspace_rest_client/client.py | Removes wildcard imports, adds typing, small refactors, and safer defaults. |
| dspace_rest_client/init.py | Replaces wildcard re-export with explicit public API exports. |
| .pre-commit-config.yaml | Adds pre-commit hooks for formatting/linting consistent with the consumer repo. |
| .github/workflows/tests.yml | Keeps pytest matrix and adds lint + non-blocking typecheck jobs via uvx. |
Suppressed comments (2)
dspace_rest_client/models.py:67
- This is a mutable class attribute (
{}), which is shared across instances if any code mutates it before__init__assigns an instance attribute. Prefer an annotation-only attribute (orNone) and initialize per-instance in__init__(which you already do).
metadata = {}
dspace_rest_client/models.py:246
- This is a mutable class attribute (
{}), which is shared across instances. Since the base initializer setsself.metadata, this can be an annotation-only attribute to avoid an unnecessary shared default at the class level.
metadata = {}
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 11 changed files in this pull request and generated 1 comment.
Suppressed comments (3)
pyproject.toml:10
- Because this README is also the published package description, its installation requirements must be updated with the migration. It still advertises Python 3.x/3.8 and tells source users to install the now-deleted
requirements.txt(README.md:13-26), so the documented install command fails and the Python requirement conflicts with>=3.10.
readme = "README.md"
requires-python = ">=3.10"
dspace_rest_client/client.py:561
- The new public annotation rejects a supported and tested call: this method deliberately treats
action=Noneas “omit the action filter” (seetests/test_client_read.py:283-297and the guard below), but thestrannotation makes typed consumers fail that call now that the package shipspy.typed. IncludeNonein the parameter type.
def get_resourcepolicy(self, uuid: str, action: str = 'READ') -> Optional[list]:
dspace_rest_client/client.py:141
- The mutable-default problem remains because every client constructed without
proxiesreceives the same class-level dictionary. Mutating one instance'sclient.proxiestherefore changes the proxy configuration of other default instances; copy the default when assigning it.
self.proxies = proxies if proxies is not None else self.PROXY_DICT
| [build-system] | ||
| requires = ["setuptools>=61.0"] | ||
| build-backend = "setuptools.build_meta" |
- get_resourcepolicy: annotate action as Optional[str]. None is a tested, supported value that omits the action filter, but the str annotation rejected it for typed consumers now that the package ships py.typed. - Copy the class-level PROXY_DICT default per instance so mutating one client's .proxies cannot leak into other default-constructed clients. - README: drop the deleted requirements.txt install step and the stale Python 3.8 requirement (now `pip install .` / Python 3.10+).
The library is consumed as a vendored submodule on sys.path, so the type hints in the source are read directly and the PEP 561 marker adds nothing; MANIFEST.in only affects an sdist that is never built here. Remove both and the now-dangling package-data entry.
Makes
dspace_rest_clienta first-class project aligned with the consumer repo's conventions (AGENTS.md,.pre-commit-config.yaml,pyproject.toml). Five threads, all behaviour-preserving — all 68 tests still pass.What changed
1. Packaging → PEP 621
pyproject.toml(replacessetup.py)requestsas the only core dep,solr+devoptional-dependency groups..py), which is how this repo consumes the library (submodule onsys.path); nopy.typed/MANIFEST.inceremony is carried for a package that is never pip-installed here.requirements.txt—fpdf/textblob/setuptoolswere unused cruft; runtime deps now live inpyproject.requirements-test.txtis kept (the consumer repo's CI installs it).2. Shared toolchain (
.pre-commit-config.yaml+[tool.*])mixed-line-ending,autopep8 --max-line-length=90,ruff,pylint --rcfile=pyproject.toml.[tool.ruff]/[tool.pylint]/[tool.autopep8]/[tool.mypy]copied and adapted from the consumer standard.3.
uv/uvxdev + CI workflowuvx(no global installs); local dev viauvx/uv run.4. CI jobs (
.github/workflows/tests.yml)0.12.7+ pylint3.3.7, versions pinned to the consumer's) and a typecheck job (mypy,continue-on-error— see typing note). Existing pytest matrix (3.10 / 3.12) retained; pip cache re-keyed offpyproject.toml.5. Lint cleanup + type hints — pylint 9.36 → 10.00 / 10, no behaviour change:
__all__(client,__init__);_is_valid_uuid(), stopped shadowing the builtinid;proxies),list()/dict()literals,no-else-return,consider-using-in,inconsistent-return-statements,logging-not-lazy,pointless-string-statement, unused vars;WorkspaceItemoverride to inheritance;from __future__ import annotations) acrossmodels.py/client.py.Validation (local, CI-exact versions)
dspace_rest_client+tests)--rcfile=pyproject.toml)uv build)Notes for the reviewer
>=3.10(was>=3.8in the oldsetup.py), and the classifiers were trimmed to match. This is intentional, not incidental: CI only tests 3.10/3.12, the consumer repo targets 3.10+, and upstream has moved to 3.13. It does not come from a runtime requirement —from __future__ import annotationskeeps the new PEP 604 (X | None) hints lazy — it simply makes the metadata reflect what we actually test and support.typecheckjob iscontinue-on-error: true. Baseline: ~54 mypy (default mode) / 289--stricterrors, to be burned down incrementally; the remaining ones are pre-existing dynamic patterns (e.g.params = Nonereassignment, optionalself.solr), not regressions from the added hints.[tool.pylint], tracked as follow-ups, not done here to keep the diff low-risk): splitting the 1482-lineDSpaceClientgod-class (too-many-lines/too-many-public-methods), and the mutable class-attribute defaults on the model classes.