refactor: use static duck typing for packages #356
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
☂️ Python Coverage
Overall Coverage
New FilesNo new covered files... Modified Files
|
tdstein
force-pushed
the
tdstein/packages-with-protocols
branch
from
fc57890 to
84fdb8d
Compare
tdstein
requested review from
schloerke and
toph-allen
and removed request for
schloerke
schloerke
approved these changes
Dec 12, 2024
|
|
||
| app_id: Required[str] | ||
| """The numerical identifier of the application this package is associated with""" | ||
| class _Packages(Sized, Protocol): |
Collaborator
There was a problem hiding this comment.
Is it possible to suffix the class names with P to help visually declare that it's a protocol class?
Suggested change
| class _Packages(Sized, Protocol): | |
| class _PackagesP(Sized, Protocol): |
(Much easier to perform with "rename symbol")
Collaborator
Author
There was a problem hiding this comment.
Actually, I think we should make these public types. External users may want to use them for type hinting.
For example...
from posit.connect import Client, Packages
client = Client()
packages: Packages = client.packages| version: Required[str] | ||
| hash: Required[Optional[str]] | ||
| @overload | ||
| def __getitem__(self, index: slice) -> _ContentPackage: ... |
Collaborator
There was a problem hiding this comment.
Suggested change
| def __getitem__(self, index: slice) -> _ContentPackage: ... | |
| def __getitem__(self, index: slice) -> list[_ContentPackage]: ... |
Comment on lines
+12
to
+16
| @overload | ||
| def __getitem__(self, index: SupportsIndex) -> _ContentPackage: ... | ||
|
|
||
| class ContentPackage(Active): | ||
| class _Package(TypedDict): | ||
| language: Required[str] | ||
| name: Required[str] | ||
| version: Required[str] | ||
| hash: Required[Optional[str]] | ||
| @overload | ||
| def __getitem__(self, index: slice) -> _ContentPackage: ... |
Collaborator
There was a problem hiding this comment.
Is it possible to put this into a generic helper class?
class _GetItem(Sized, Generic[T], Protocol):
@overload
def __getitem__(self, index: SupportsIndex) -> T: ...
@overload
def __getitem__(self, index: slice) -> list[T]: ...
Merged
Co-authored-by: Barret Schloerke <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.