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.
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
Add cloud event to core #16800
Add cloud event to core #16800
Changes from 10 commits
abe2389
9ed935d
d11e02f
ec3474c
e658ce7
50de6e6
9f3624b
04acd47
f18f35d
9400e0e
70e08c0
b69cd73
89c80b5
10d81c3
a121adc
428e35c
c88c1e9
f638961
13335c1
2dec996
c3368d5
f461890
248db3d
32b2532
2441222
666bbd7
45d2a90
8c1f9fc
8dcf54c
f0d718f
950ffed
c15a73a
a756fe3
6b4a31f
9371f77
78696ef
79e74d4
a9c05f4
57b1bd0
e8b53b0
3a16ca9
37e887c
8e196ef
b4c726e
3f49138
22db8b8
6e9ce14
43a79c1
ba654a0
4f9d80e
0a3aa87
5051034
c7afd6e
9613fd4
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
black the file :)
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.
Please make it to _utils, and use it if parsing show
Z
, which will make the CloudEvent roudtrippable. Otherwise you send TZ_UTC class, but re-parsing back will give a FixedOffset(0) instance.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 quite understand this comment. How do I use data_base64 of a CloudEvent instance?
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.
this doesn't apply anymore - removed the line
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 "will be sent as..." comment applies to the event grid sender client, right?
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 can technically be any request
"will be sent as data_base64 in the outgoing request." is the wording 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.
But there is no code here that will do base64 encoding of the data in this type? I would have understood it better if there was a
to_dict
method that would convertself.data
to'data_base64': <base 64 encoded self.data>
.In other words, are you trying to document a requirement for the consumer of this data 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.
i'm trying to docuement the implementation detail of json serialization
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.
Optional[Dict]
?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.
**Any
.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.
We are missing type annotations for all instance variables that we
pop
fromkwargs
(sincekwargs
is typed as**Any
). We should add them or else static type checkers (e.g. MyPy) will not be able to reason about the type...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 would expect that we raise an error for unexpected (remaining)
kwargs
.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 am not sure it's a good idea - wouldn't this prevent us from having additive changes in the future?
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.
How would this prevent us from having additive changes in the future?
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.
hmm, i was thinking on the lines of "Aren't we blocking adding keyword args in the future", but i got it wrong - updated
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 purpose of
**kwargs
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.
no real purpose really - removed it for now - it can be additive
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 annotation is incorrect - you will return an instance of cls. Off the top of my head, this would be:
# type: (Type[T], Dict, **Any) -> T
where T is a typing.TypeVar (ideally bound to
CloudEvent
)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 we want to mutate the argument passed in. I would be a bit surprised to notice that my dict had lost (all of its) keys when the method returned.
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.
changed this
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 are using
time
below - this will generate aUnboundLocalError
if you hit this line.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.
fixed it
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 happens if
data
is''
?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.
changed this a lil bit - data '' is accepted