Modernize packaging #193
Modernize packaging #193
Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Marking this as draft to make the upstream changes in sdk-generator, I'll force push on this branch. |
|
Hey @abhiaagarwal 👋 Thanks a lot for contributing this! Now that the upstream Generator changes are merged, would you be up for updating this to bring the repo in sync? |
|
@evansims absolutely! |
Signed-off-by: Abhi Agarwal <[email protected]>
abhiaagarwal
force-pushed
the
modernize-package
branch
from
95803ec to
69979bf
Compare
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis change migrates the Python SDK project from legacy packaging and dependency management using Changes
Sequence Diagram(s)sequenceDiagram
participant GitHub Actions
participant uv
participant Python Environment
GitHub Actions->>uv: Setup Python (setup-uv)
uv->>Python Environment: Create environment, install dependencies (uv sync)
GitHub Actions->>uv: Run tests/lint/build (uv run, uv build)
uv->>Python Environment: Execute commands in managed environment
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (2)
pyproject.toml (1)
139-144: mypy section contains invalid keys & list-valuedexclude
packagesis not a recognised mypy config option, andexcludeexpects a single regex string, not an array. Current config will cause “Unknown configuration option” errors and silently ignore the intended exclude.-[tool.mypy] -python_version = "3.10" -packages = "openfga_sdk" -exclude = [ - "openfga_sdk/models", -] +[tool.mypy] +python_version = "3.10" +# Match anything under openfga_sdk/models +exclude = "(^|/)openfga_sdk/models/"openfga_sdk/sync/open_fga_api.py (1)
1308-1316: Update test invocations to remove thenameparameter
Thenameargument was removed fromlist_stores_with_http_infoand now passing it will raiseFgaValidationException. Existing tests still calllist_stores(name=…), so they must be updated:• test/sync/open_fga_api_test.py:474
• test/api/open_fga_api_test.py:1892Please remove (or replace) the
name="test-store"argument in these calls and adjust assertions to match the new behavior.
🧹 Nitpick comments (4)
pyproject.toml (2)
37-42: Consider pinning upper bounds for all runtime depsOnly
urllib3has an explicit<3cap; the other three libraries can introduce breaking API changes in any major release. Adding upper-bound pins (e.g.<4) avoids accidental breakage for your consumers.
47-55:[dependency-groups]is hatch-specific – document itReaders unfamiliar with Hatch will not immediately know how to install the dev group (
uv pip install -e . -G dev,hatch env create, etc.). A short comment in this stanza or in CONTRIBUTING/README will prevent onboarding friction..github/workflows/main.yaml (1)
70-77: Cache key should includeuv.lock/ resolved deps
cache-dependency-glob: "**/pyproject.toml"ignores the lock-file generated byuv pip compile.
Ifuv.lock(or.venv.json) changes butpyproject.tomlstays the same, CI may reuse a stale cache.cache-dependency-glob: | **/pyproject.toml **/uv.lockdocs/OpenFgaApi.md (1)
643-651: Example call still usesapi_instance.api_instance; drop the redundant attributeThe SDK client returned by
openfga_sdk.OpenFgaApi(...)already exposes the endpoint methods.
Usingapi_instance.api_instance.list_stores(...)in the snippet will raise anAttributeErrorfor real code and may confuse users. Update the example to a singleapi_instance:- api_response = await api_instance.api_instance.list_stores( - page_size=page_size, - continuation_token=continuation_token - ) + api_response = await api_instance.list_stores( + page_size=page_size, + continuation_token=continuation_token + )(No further issues with the removal of the obsolete
nameparameter were found.)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
.github/workflows/main.yaml(2 hunks)README.md(1 hunks)docs/OpenFgaApi.md(1 hunks)openfga_sdk/api/open_fga_api.py(1 hunks)openfga_sdk/sync/open_fga_api.py(1 hunks)pyproject.toml(2 hunks)requirements.txt(0 hunks)setup.py(0 hunks)test-requirements.txt(0 hunks)
💤 Files with no reviewable changes (3)
- requirements.txt
- test-requirements.txt
- setup.py
🔇 Additional comments (1)
openfga_sdk/api/open_fga_api.py (1)
1314-1314: LGTM! Parameter removal properly implemented.The removal of the
nameparameter from theall_paramslist correctly implements the API change described in the PR summary. This change is consistent with the method documentation and aligns with the coordinated updates across async/sync clients.
|
Hmmm, it seems that #202 needs to be updated upstream as well |
|
Thanks @abhiaagarwal! I've got a PR created on the Generator to bring it up-to-date with 202 now. |
|
👋 @abhiaagarwal, we just merged that PR. Sorry for the delay; we had to fix some tooling on the Generator repo to unblock that. You should be good to go now! |
Description
This migrates the old setuptools-based packaging to instead use the
pyproject.tomlformat, withuvas the package manager andhatchlingas the build system. This mainly avoids adding the transitive dependencies ofbuildandsetuptoolsthat are currently packaged with the sdk.Moves CI over to use
uv.References
Closes #192
Review Checklist
mainSummary by CodeRabbit
New Features
Bug Fixes
Documentation
namefilter from thelist_storesmethod.Refactor
nameparameter in thelist_storesAPI client methods.Chores