adding stubs for passpy API - #6651
Merged
Merged
Conversation
Member
|
You can just use |
Following recommendations from JelleZijlstra.
Contributor
Author
|
How nice. This looks better already. Thanks. |
| ) -> None: ... | ||
| def __iter__(self) -> Iterator[str]: ... | ||
| def is_init(self) -> bool: ... | ||
| def init_store(self, gpg_ids: Iterable[str], path: str | None = ...) -> None: ... |
Member
There was a problem hiding this comment.
The code has
# Ensure that gpg_ids is a list so that the later .join does
# not accidentally join single letters of a string.
if gpg_ids is not None and not isinstance(gpg_ids, list):
gpg_ids = [gpg_ids]
So I think it can be None, str, or list[str], but not any Iterable.
JelleZijlstra
requested changes
Dec 22, 2021
JelleZijlstra
left a comment
JelleZijlstra
left a comment
Member
There was a problem hiding this comment.
Thanks! I have a couple of comments in addition to the inline ones:
passpy.util.gen_passwordalso seems to be public, could you add it?- I think many of the path argument should work with any PathLike object, because they just get passed to
os.path. You could use_typeshed.StrPathto annotate them. - I noticed you use
MappingandIterablefor some return types even though the implementation uses concrete types like dict and list. We generally prefer using concrete types in return annotations.
| def is_init(self) -> bool: ... | ||
| def init_store(self, gpg_ids: Iterable[str], path: str | None = ...) -> None: ... | ||
| def init_git(self) -> None: ... | ||
| def git(self, method: str, *args: Iterable[object], **kwargs: Mapping[str, object]) -> None: ... |
Member
There was a problem hiding this comment.
Suggested change
| def git(self, method: str, *args: Iterable[object], **kwargs: Mapping[str, object]) -> None: ... | |
| def git(self, method: str, *args: object, **kwargs: object) -> None: ... |
Annotations on *args and **kwargs refer to individual element, not the whole object.
| def init_store(self, gpg_ids: Iterable[str], path: str | None = ...) -> None: ... | ||
| def init_git(self) -> None: ... | ||
| def git(self, method: str, *args: Iterable[object], **kwargs: Mapping[str, object]) -> None: ... | ||
| def get_key(self, path: str) -> str: ... |
Member
There was a problem hiding this comment.
Suggested change
| def get_key(self, path: str) -> str: ... | |
| def get_key(self, path: str | None) -> str | None: ... |
It can return None. It may be useful to add an overload to indicate that it only returns None if the argument is None or the empty string.
| def init_git(self) -> None: ... | ||
| def git(self, method: str, *args: Iterable[object], **kwargs: Mapping[str, object]) -> None: ... | ||
| def get_key(self, path: str) -> str: ... | ||
| def set_key(self, path: str, key_data: str, force: bool = ...) -> None: ... |
Member
There was a problem hiding this comment.
Suggested change
| def set_key(self, path: str, key_data: str, force: bool = ...) -> None: ... | |
| def set_key(self, path: str | None, key_data: str, force: bool = ...) -> None: ... |
This library is really into accepting None everywhere.
| def get_key(self, path: str) -> str: ... | ||
| def set_key(self, path: str, key_data: str, force: bool = ...) -> None: ... | ||
| def remove_path(self, path: str, recursive: bool = ..., force: bool = ...) -> None: ... | ||
| def gen_key(self, path: str, length: int, symbols: bool = ..., force: bool = ..., inplace: bool = ...) -> str | None: ... |
Member
There was a problem hiding this comment.
Suggested change
| def gen_key(self, path: str, length: int, symbols: bool = ..., force: bool = ..., inplace: bool = ...) -> str | None: ... | |
| def gen_key(self, path: str | None, length: int, symbols: bool = ..., force: bool = ..., inplace: bool = ...) -> str | None: ... |
| def copy_path(self, old_path: str, new_path: str, force: bool = ...) -> None: ... | ||
| def move_path(self, old_path: str, new_path: str, force: bool = ...) -> None: ... | ||
| def list_dir(self, path: str) -> tuple[Iterable[str], Iterable[str]]: ... | ||
| def iter_dir(self, path: str) -> None: ... |
Member
There was a problem hiding this comment.
Suggested change
| def iter_dir(self, path: str) -> None: ... | |
| def iter_dir(self, path: str) -> Iterator[str]: ... |
It's a generator
| def move_path(self, old_path: str, new_path: str, force: bool = ...) -> None: ... | ||
| def list_dir(self, path: str) -> tuple[Iterable[str], Iterable[str]]: ... | ||
| def iter_dir(self, path: str) -> None: ... | ||
| def find(self, names: Iterable[str]) -> Iterable[str]: ... |
Member
There was a problem hiding this comment.
Suggested change
| def find(self, names: Iterable[str]) -> Iterable[str]: ... | |
| def find(self, names: None | str | list[str]) -> list[str]: ... |
Another one that does
if not isinstance(names, list):
names = [names]
added 2 commits
December 22, 2021 11:06
* using _typeshed.StrPath rather that str when possible * switching to concrete return types annotations * adding passpy.util.gen_password since this can considered part of the passpy API * fixing additional parameter annotations references: * #6651 (comment) * #6651 (review) acknowledgement: * thanks to JelleZijlstra for their review
Co-authored-by: JelleZijlstra
Contributor
Author
|
Okay, I think I addressed all your comments. |
JelleZijlstra
approved these changes
Dec 22, 2021
JelleZijlstra
left a comment
JelleZijlstra
left a comment
Member
There was a problem hiding this comment.
Just one more thing, thanks! I'll apply the change and then merge this once CI passes.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
adding stubs for passpy: https://pypi.org/project/passpy/
I had to replace
re.MatchbyAnyfor python 3.6 compatibility. I'm not sure this is the right way to do this.