-
Notifications
You must be signed in to change notification settings - Fork 58
CloudEvent class needs better access to cloudevent fields #49
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
Comments
I'm not sure that you want attribute accessors, given that the set of CE attributes are unknown a priori due to the possible presence of extensions. Using attribute access may also make iterating over all the attributes, or adding additional methods on the class more difficult. |
I was thinking about providing an interface from CloudEvent to the existing python properties within the base event class such as: cloudevent = CloudEvent(attributes, data)
cloudevent.event.specversion I don't want access properties to the _attribute field if that's what you thought I meant. I acknowledge this will take a bit of refactoring/design decision rework if we agreed to go this route because we'd likely have to instantiate some form of events during the constructor call which will require discussion. For extensions, which are unknown, you can access them through the above example by doing: cloudevent.event.extensions['key'] This naming schema feels a tad bit inconvenient so perhaps we can make adjustments here but overall I think it would be nice for users to be able to auto complete fields through python properties, specifically for the required and optional cloudevent metadata fields. It would reduce obscurity during event consumption and make code slightly more verbose in my opinion. Aside from that we will want to revise the README to have example usage of the getitem overload. I sort of want to overhaul the README in general. I don't think we want to recommend users to use the base event classes atm. I'll issue a follow up PR on README changes but lmk your thoughts on the above @evankanderson |
All the old base stuff needs to disappear or at least be hidden and not recommended for external use. You'll note that I didn't hold on to the base event in One thing I would try to do before declaring a
|
I'm -1 on having a sub-attribute named If we want to do attribute access, they should just be top-level properties, since the name If you do want to be able to use attribute access instead of map-style access, I think the following should be sufficient:
|
The point on autocomplete is interesting -- I haven't looked much at how to hint that in various editors. |
I'm also -1 on the event name also hence me saying the naming schema I gave above sounds inconvenient. Top level properties rather than nested objects is definitely preferable although marginally more annoying to implement. I further agree we should keep the getitem overload regardless. The [a-z0-9] naming schema is an interesting point I was unaware of. I would like to know why the spec permits this anyways, although there are no existing attributes where the first character is a number that I know of. I also agree the http function handles you described should be separate from the event itself. Perhaps we should break this issue up until several smaller issues. |
What about just adding |
Also Java or Go sdk allow dot access on the known attributes. |
@ace-n also mentions a |
Expected Behavior
Actual Behavior
Steps to Reproduce the Problem
The text was updated successfully, but these errors were encountered: