Skip to content
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

sdk: python: Fix inconsistencies in passing IDs to fields #4195

Merged
merged 1 commit into from Dec 15, 2022

Conversation

helderco
Copy link
Contributor

@helderco helderco commented Dec 14, 2022

Fixes #4191

Breaking change

  • Container.with_rootfs(id) no longer accepts a DirectoryID. Only a Directory object.
  • Top level fields that have an id argument and return its object, no longer accept an object as argument. Only an ID.
- def file(id: FileID | File) -> File
+ def file(id: FileID) -> File

See PR's diff for all changes.

@helderco helderco added the breaking change Breaking Change label Dec 14, 2022
@helderco helderco force-pushed the python/codegen branch 3 times, most recently from 9d1fb4b to 6fc1f2c Compare December 14, 2022 14:05
@helderco
Copy link
Contributor Author

Rebased.

@helderco helderco force-pushed the python/codegen branch 2 times, most recently from 82e6175 to 11339c9 Compare December 14, 2022 14:51
Signed-off-by: Helder Correia <174525+helderco@users.noreply.github.com>
Copy link
Member

@grouville grouville left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works as expected 😇

Tested locally on local engine (built it + pushed it to local registry) + local SDK

Top level fields now accept ids, it's so much clearer !
If:
-> you want to create a directory from scratch, you either specify nothing or a directoryID.
-> You want to append a dir to a container, you add directly a dir

It makes a lot of sense and fits well with the global API.

Example of:

  1. creation of a directory from a directoryID
  2. mount of the directory on the python container

image

Coupled with the runtime checks, it works well. Here, trying to use the id to add the dir to the container:
image
It fails, as expected, with the proper error message

LGTM, tested on most of the changes present on API. Great DX improvement 👏

@grouville grouville merged commit 5bba71a into dagger:main Dec 15, 2022
@helderco helderco deleted the python/codegen branch December 15, 2022 14:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Breaking Change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Python: Don’t accept an object for parameters named id (and the return is of the same type)
2 participants