Make pyright happy #152
Make pyright happy #152
Conversation
Signed-off-by: poksiala <[email protected]>
| def get_auth_header(self): | ||
| raise NotImplementedError | ||
|
|
||
| def get_container(self, name: container_type) -> oras.container.Container: |
There was a problem hiding this comment.
I removed this method as it relies on self.hostname which is not available
| @decorator.ensure_container | ||
| def load_configs(self, container: container_type, configs: Optional[list] = None): | ||
| def load_configs( | ||
| self, container: oras.container.Container, configs: Optional[list] = None |
There was a problem hiding this comment.
I changed the container type as it relied on self.get_container() method above trough the decorator. Which as said would not have worked anyways
| @@ -120,37 +133,3 @@ def set_basic_auth(self, username: str, password: str): | |||
| :type password: str | |||
| """ | |||
| self._basic_auth = auth_utils.get_basic_auth(username, password) | |||
|
|
|||
| def request_anonymous_token(self, h: auth_utils.authHeader, headers: dict) -> bool: | |||
There was a problem hiding this comment.
To my understanding this is dead code and completely incompatible with the overriding method in token auth, which is actually being used.
| raise NotImplementedError | ||
|
|
||
| def get_container(self, name: container_type) -> oras.container.Container: | ||
| def set_header(self, name: str, value: str): |
There was a problem hiding this comment.
This method was referenced in multiple places but did not actually exist. So I added it by copying it from Provider.
Signed-off-by: Panu Oksiala <[email protected]>
oras/auth/token.py
Outdated
| if not h.realm.startswith("http"): # type: ignore | ||
| h.realm = f"{self.prefix}://{h.realm}" | ||
| if not h.realm.startswith("http"): | ||
| h.realm = f"http://{h.realm}" # TODO: Should this be htts |
| @@ -120,10 +122,9 @@ def delete_tags(self, name: str, tags=Union[str, list]) -> List[str]: | |||
| :param tags: single or multiple tags name to delete | |||
| :type N: string or list | |||
| """ | |||
| if isinstance(tags, str): | |||
| tags = [tags] | |||
| _tags = [tags] if isinstance(tags, str) else tags | |||
There was a problem hiding this comment.
Why did you make these variables private?
There was a problem hiding this comment.
This is a bit useless change as the real issue in this method was that there was assignment tags=Union[str, list] instead of of type hinting tags: Union[str, List[str]] in the signature. I did not notice that at first and thought that the previous if statement was not clear enough for current type checkers.
On the other hand I personally prefer this as it is clear that _tags might be or might not be same as tags given as an argument. it could be named as tag_list or something as well.
Not sure which is the most pythonic way to go as I do most of my coding in languages with immutable variables and as such assigning list to string feels a bit dirty.
Signed-off-by: Panu Oksiala <[email protected]>
|
@poksiala apologies this fell off my radar - if you want to rebase I can give another review and (I think) it should be ready for merge. |
I tried to use this library and to be honest the user experience was not the best. For me it was hard to understand with all the missing and invalid type annotations. My editor was also complaining a lot, especially about the decorated methods that did not pass trough information about the methods.
So as an exercise to better understand the inner workings of the library I started to fix those type annotations. I also found some dead code and missing variables while doing that. Here are the results if you are interested in including them in the trunk.
This should not change the API at all though I have had no time to test it in actual use yet.