-
Notifications
You must be signed in to change notification settings - Fork 22
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
Manifest creation helper #1103
Manifest creation helper #1103
Conversation
as mentioned in the original issue: #1090 the ultimate goal here is to have a helper function able to generate a valid manifest with minimal, obligatory input, e.g.:
|
4e6e54c
to
57f171c
Compare
else: | ||
from typing_extensions import Literal | ||
|
||
from dateutil.tz import UTC |
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.
hmmm, isn't this the same as datetime.timezone.utc
?
homepage: Optional[str] = None | ||
description: Optional[str] = None | ||
|
||
def dict(self, *, by_alias=False): |
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.
can't we use dataclasses.asdict
here?
homepage: Optional[str] = None | ||
description: Optional[str] = None | ||
|
||
def dict(self, *, by_alias=False): |
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.
plus, I believe you're not using the by_alias
... again, no warnings from linters?
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.
@approxit I believe the implementation is mostly as good as it can be given the use of plain dataclasses ... I have added a couple of questions/suggestions
the important ones concern the retrieval of an external network resource in unit tests - I'd very much like those to be mocked ...
plus, am I missing something or does this pull request still contain no helper mentioned in the original task?
cls, image_hash: str = None, outbound_urls: List[str] = None, **kwargs | ||
) -> "Manifest": | ||
if image_hash is not None: | ||
kwargs["payload.0.hash"] = image_hash |
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.
it's almost perfect but the manifest, afaik, must also contain the image url ;)
ea1dc5d
to
c0e5c9c
Compare
c0e5c9c
to
5398306
Compare
What I've done:
.dict(by_alias)
and.parse_obj(obj)
methods that reflects methods found inpydantic
as this kind of models are justpydantic
aim.parse_imploded_obj(obj)
to ease with providing nested structuresAs this kind of DTOs should be implemented in pydantic, this should be mentioned as work item in golemfactory/golem-core-python#54.
Keep in mind, that parsing object is quite a complex task. This implementation is not mentioned to be so robust as pydantic's.
.parse_obj(obj)
is just "base minimum".Also exploding nested dicts is very easy task, but supporting exploding dicts with lists... oh boy, code complexity explodes too... I'm not very happy how it turned out, but at least its working and its covered with tests.