Skip to content

Improve docker.types #13809

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Conversation

donBarbos
Copy link
Contributor

No description provided.

This comment has been minimized.

@Avasam Avasam self-assigned this Apr 10, 2025
Copy link
Collaborator

@Avasam Avasam left a comment

Choose a reason for hiding this comment

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

I didn't comment it on every instance, but the annoying thing with using list and dict as parameter types, is that they're invariant. Meaning unions have to be exact.
A list[str] cannot be passed to a list[str | int] parameter !

So everywhere you have a union like dict[str, Foo | Bar], you need to expand it to accept all possible permutations:
dict[str, Foo] | dict[str, Bar] | dict[str, Foo | Bar] (same with lists)

That's why, even though some methods not explicitly requiring a list or dict may be an implementation detail, I instead recommended some covariant types like Iterable and Mapping where possible.

This comment has been minimized.

This comment has been minimized.

@donBarbos
Copy link
Contributor Author

Looks done 👍
I used Iterable as you suggested, but I wanted to ask what you think about using Sequence, it seems to be a more strict and more suitable for list parameters

@donBarbos donBarbos requested a review from Avasam April 13, 2025 14:00
@Avasam
Copy link
Collaborator

Avasam commented Apr 14, 2025

I used Iterable as you suggested, but I wanted to ask what you think about using Sequence, it seems to be a more strict and more suitable for list parameters

Hmm... I'm not hard set on either. Iterable/Container is exact to the actual implementation, but it also looks like an implementation detail that we can get away with non-lists in those cases, so maybe it's "safer" for the users to use a Sequence.

This is also something we can ask about upstream to get some clarification.

My personal preference is to follow the implementation, and update it in a version bump PR if it changes. But I won't block on either.

@Avasam
Copy link
Collaborator

Avasam commented Apr 14, 2025

Using GitHub comments was a bit too complex to list the rest of the invariance issues, so I opened donBarbos#1 against your PR branch.

@donBarbos
Copy link
Contributor Author

Big thank to you. In the future I'll try to be more careful with invariant types

Copy link
Contributor

Diff from mypy_primer, showing the effect of this PR on open source code:

dragonchain (https://github.com/dragonchain/dragonchain)
- dragonchain/job_processor/contract_job.py:251:16: error: Module has no attribute "errors"  [attr-defined]
- dragonchain/job_processor/contract_job.py:268:17: error: Module has no attribute "errors"  [attr-defined]
- dragonchain/job_processor/contract_job.py:280:16: error: Module has no attribute "errors"  [attr-defined]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants