-
Notifications
You must be signed in to change notification settings - Fork 6
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
Main api #53
Main api #53
Conversation
Instead of having logic in the cli module, have a proper api module that is exposed directory on the awspub module level. That way, the api can be used simply with: > import awspub > awspub.create() > awspub.public() and so on.
This is actually only moving code from cli/init.py to the new api.py . and then increase test coverage and add documentation. and obviously use the api from the cli module. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
overall lgtm. couple nits
With that, it's easier to understand which API should actually be used when integrating the awspub python module.
To increase test coverage and make sure that no regression will be introduced.
Use an actionable name for the publication step to make the action more obvious.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mypy
strict gives me:
awspub/api.py:114: error: Function is missing a return type annotation [no-untyped-def]
awspub/api.py:131: error: Function is missing a return type annotation [no-untyped-def]
awspub/api.py:148: error: Missing type parameters for generic type "Dict" [type-arg]
awspub/api.py:161: error: Missing type parameters for generic type "Dict" [type-arg]
Found 4 errors in 1 file (checked 1 source file)
But you haven't configured mypy with --strict
in tox.ini
so I don't know if you want to fix that.
|
||
def _images_grouped( | ||
images: List[Tuple[str, Image, Dict[str, _ImageInfo]]], group: Optional[str] | ||
) -> Tuple[Dict[str, Dict[str, str]], Dict[str, Dict[str, Dict[str, str]]]]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could define your custom types here. Like:
type ImageMetadata = Dict[str, str]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
type
is >= python 3.12 (see https://docs.python.org/3/reference/simple_stmts.html#type ). but we want to support 3.10 (from jammy) for now.
:param group: a optional group name | ||
:type group: Optional[str] | ||
:return: the images grouped by name and by group | ||
:rtype: Tuple[Dict[str, Dict[str, str]], Dict[str, Dict[str, Dict[str, str]]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this needed, it's already in the signature above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like there is https://github.com/tox-dev/sphinx-autodoc-typehints that might help here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Filled #56 to explore this more.
Created #55 to enable |
No description provided.