-
Notifications
You must be signed in to change notification settings - Fork 56
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
refactor: create abstract cloudevent #186
refactor: create abstract cloudevent #186
Conversation
…udevents#171) Signed-off-by: Alexander Tkachev <sasha64sasha@gmail.com>
Signed-off-by: Alexander Tkachev <sasha64sasha@gmail.com>
Signed-off-by: Alexander Tkachev <sasha64sasha@gmail.com>
Signed-off-by: Alexander Tkachev <sasha64sasha@gmail.com>
Signed-off-by: Alexander Tkachev <sasha64sasha@gmail.com>
Signed-off-by: Alexander Tkachev <sasha64sasha@gmail.com>
Signed-off-by: Alexander Tkachev <sasha64sasha@gmail.com>
for more information, see https://pre-commit.ci
Signed-off-by: Alexander Tkachev <sasha64sasha@gmail.com>
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
Signed-off-by: Alexander Tkachev <sasha64sasha@gmail.com>
Signed-off-by: Alexander Tkachev <sasha64sasha@gmail.com>
Signed-off-by: Alexander Tkachev <sasha64sasha@gmail.com>
Signed-off-by: Alexander Tkachev <sasha64sasha@gmail.com>
Signed-off-by: Alexander Tkachev <sasha64sasha@gmail.com>
Signed-off-by: Alexander Tkachev <sasha64sasha@gmail.com>
Signed-off-by: Alexander Tkachev <sasha64sasha@gmail.com>
Signed-off-by: Alexander Tkachev <sasha64sasha@gmail.com>
Signed-off-by: Alexander Tkachev <sasha64sasha@gmail.com>
Signed-off-by: Alexander Tkachev <sasha64sasha@gmail.com>
Signed-off-by: Alexander Tkachev <sasha64sasha@gmail.com>
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.
@sasha-tkachev thx for your PR. There are some good ideas in this approach, but I don't get some of the decisions being made. Please see some comments and suggestions noted below.
cloudevents/abstract/event.py
Outdated
@property | ||
@abc.abstractmethod | ||
def _attributes_read_model(self) -> typing.Dict[str, typing.Any]: | ||
""" | ||
:return: attributes of this event | ||
|
||
you MUST NOT mutate this dict | ||
implementation MAY assume the dic will not be mutated | ||
""" | ||
pass |
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 don't we go with public attributes
property? This way each implementation will expose the attributes which may be beneficial.
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 is to prevent confision. Documented in the function this reason
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.
What kind of confusion? We're basically talking about having an interface that exposes desired public APIs.
It is beneficial to discuss which properties must be exposed and which not.
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.
* What is the difference between `event.get("myattr")` and
`event.attributes.get("myattr")`
* What SHOULD I use `event["myattr"]` or `event.attributes["myattr"]` ?
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.
I'm not saying that one should not use .get way. I'm saying that it may be beneficial to expose attributes sometimes.
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.
OK fine with me
cloudevents/abstract/event.py
Outdated
@property | ||
@abc.abstractmethod | ||
def _data_read_model(self) -> typing.Optional[typing.Any]: | ||
""" | ||
:return: data value of the event | ||
you MUST NOT mutate this value | ||
implementation MAY assume the value will not be mutated | ||
""" | ||
pass |
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.
The same question here. I'd rather have it here as just data
and hide data
field in the implementation.
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.
I can't use the data
property because in pydantic i need to declare it as a field and not a property
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.
Well, I'm not sure that's a valid reason to drop this approach. Maybe worth naming data
differently in Pydantic model instead?
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.
but then it will be serialized funky in json form
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.
i think maybe we should define the cloudevent as protocol
this will make much more sense dont you think?
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.
Protocol would be nice, but it's only available in python 3.8+.
And we currently support 3.6+.
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.
but then it will be serialized funky in json form
I guess it may be configured. E.g. by using alias for the data.
E.g. name the field "payload" and use data as alias
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.
good idea, i will do that
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.
did not work, see below, i found a better way
cloudevents/abstract/event.py
Outdated
pass | ||
|
||
def __setitem__(self, key: str, value: typing.Any) -> None: | ||
raise NotImplementedError() |
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 self._data_read_model()[key]=value
?
I don't really get why we want to have a read
model.
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 MAY not be possible to do so.
For example - if my cloudevent is a database row, or a cached value.
I can read the value, but setting it MAY not be possible in the same way.
updated the docs so it will be more clear
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.
Again, it is up to us to define the contract. I'd rather have the default implementation here and rely on whatever we present in the contract.
It's kinda the implementation problem of how to present the data.
While we're kinda obliged to follow the existing API till we move to v2, I'd rather keep it simple and leave it to the implementation to follow.
I'd probably go with a fully immutable class in v2 and then define some mutation helpers, but that's a different story.
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 pretty simple to read one way and write another way
its CQRS.
if you wish to change it, i can
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.
I don't think this is cqrs. Haven't ever heard of using it in the context of a single entity.
We may not provide a mutation API at all at this level.
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.
I think that you are right. i will remove the del and set from this level
cloudevents/abstract/json_methods.py
Outdated
from cloudevents.abstract.http_methods import to_structured, from_http | ||
|
||
|
||
def to_json( |
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.
I'd say it worth moving these http_methods
and json_methods
to the cloudevents
package level. These have nothing "abstract` in them. They don't belong 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.
Isn't it confusing?
what is the difference between cloudevents.http.http_methods
and cloudevents.http_methods
?
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.
Well, there's somewhat confusing APIs all over the package. But these are definitely not abstract. They look more like generic or standard implementations.
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.
Maybe even name the file with these APIs as conversion.py
or convertor.py
. This will then make a bit more sense.
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.
Maybe we should rename the whole module to generic
?
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.
Generic is a bad name for a module IMO
Signed-off-by: Alexander Tkachev <sasha64sasha@gmail.com>
Signed-off-by: Alexander Tkachev <sasha64sasha@gmail.com>
Signed-off-by: Alexander Tkachev <sasha64sasha@gmail.com>
Signed-off-by: Alexander Tkachev <sasha64sasha@gmail.com>
Signed-off-by: Alexander Tkachev <sasha64sasha@gmail.com>
Signed-off-by: Alexander Tkachev <sasha64sasha@gmail.com>
Signed-off-by: Alexander Tkachev <sasha64sasha@gmail.com>
Signed-off-by: Alexander Tkachev <sasha64sasha@gmail.com> # Conflicts: # CHANGELOG.md # cloudevents/http/event.py # cloudevents/tests/test_http_cloudevent.py
Signed-off-by: Alexander Tkachev <sasha64sasha@gmail.com>
Signed-off-by: Alexander Tkachev <sasha64sasha@gmail.com>
This reverts commit 89d30eb. Signed-off-by: Alexander Tkachev <sasha64sasha@gmail.com>
Signed-off-by: Alexander Tkachev <sasha64sasha@gmail.com>
Signed-off-by: Alexander Tkachev <sasha64sasha@gmail.com>
Signed-off-by: Alexander Tkachev <sasha64sasha@gmail.com>
Signed-off-by: Alexander Tkachev <sasha64sasha@gmail.com>
Signed-off-by: Alexander Tkachev <sasha64sasha@gmail.com>
Signed-off-by: Alexander Tkachev <sasha64sasha@gmail.com>
684a62c
to
065ef91
Compare
Signed-off-by: Alexander Tkachev <sasha64sasha@gmail.com>
@xSAVIKx i tried to implement the alias trick but pydantic does not like it at all super simple - simple, polymorphic and implementation independent Also i dont need to think about the mutability of the resulted attributes |
Signed-off-by: Alexander Tkachev <sasha64sasha@gmail.com>
Signed-off-by: Alexander Tkachev <sasha64sasha@gmail.com>
we MUST NOT assume how the __init__ function looks across different python | ||
frameworks | ||
""" | ||
raise NotImplementedError() |
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 don't we mark this method as abstract?
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.
because it causes issues in the code coverage tests.
I cannot trigger the line to be called because an abstract method call fails before the function is called
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.
Yeah, sure, but that's not a valid reason to make smth not abstract. You can just create a test no-op or error-throwing implementation directly in the test case.
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.
I tried testing the code directly but it did not actually executed the pass
line of the function
@abstractmethod
def dostuff():
pass
because the abstract method raised an exception before reaching the pass.
I will do the thing that is mentioned here
https://stackoverflow.com/questions/9202723/excluding-abstractproperties-from-coverage-reports
cloudevents/abstract/event.py
Outdated
@classmethod | ||
def get_attributes(cls, event: "CloudEvent") -> typing.Dict[str, typing.Any]: | ||
""" | ||
:return: Attributes of this event. | ||
|
||
You MUST NOT mutate this dict. | ||
Implementation MAY assume the dict will not be mutated. | ||
""" | ||
raise NotImplementedError() | ||
|
||
@classmethod | ||
def get_data(cls, event: "CloudEvent") -> typing.Optional[typing.Any]: | ||
""" | ||
:return: Data value of the event. | ||
You MUST NOT mutate this dict. | ||
Implementation MAY assume the dict will not be mutated. | ||
""" | ||
raise NotImplementedError() |
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 have classmethod? What's the benefit?
Why can't it be just an abstract method?
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 is my way of handling the pydantic edge case for the data field. I tried making an alias but it messes everything up.
I don't see any reason for forcing any data attributes or member functions for our concrete event type.
All I want is the generic access of the values im interested in especially if this is for a internal use only.
I think I may make it private to prevent confusion.
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.
I still don't get the reasoning. Just `my way of handling is not a good enough reason.
I don't see a reason why get_data(event)
can be anyhow different from event.get_data
. The latter one looks and feels more natural. Moreover, it is the event instance behavior to return some part of its state to the caller. Even if we make these APIs private (with __
) or protected (with _
), they still look very much like some abstract instance APIs, not class-level ones.
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.
And also, why don't we return a copy of the internal state instead of stating that one MUST NOT
mutate the state? (that's in case we want this to be part of the public API).
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.
I have no problem to create protected event._get_data and event._get_attributes.
this will solve the confusion i was afraid of
I thought about copying the data, but was afraid of the efficiency implication though it may be premature optimization.
@sasha-tkachev please let me know if you'd like to continue on this one. I can jump in if needed. |
@xSAVIKx It's ok, I have a-lot of motivation for this project. Just did not have enough free time for it in the past few days. |
@xSAVIKx waiting for your answers |
@sasha-tkachev I'm sorry for the late reply. Somehow I missed the notifications. You can also find me in CNCF Slack workspace if you need to poke me. Join here: https://communityinviter.com/apps/cloud-native/cncf |
…ions instead of classmethods Signed-off-by: Alexander Tkachev <sasha64sasha@gmail.com>
Signed-off-by: Alexander Tkachev <sasha64sasha@gmail.com>
for more information, see https://pre-commit.ci
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.
@sasha-tkachev overall LGTM. I've modified the docs to be a bit more reader-friendly.
Signed-off-by: Yurii Serhiichuk <savik.ne@gmail.com>
for more information, see https://pre-commit.ci
dab07d2
to
26e551b
Compare
Changes
Created an abstract cloudevent class to be used in #182