refactor: use CloudEvents not cloudevents everywhere #101
Conversation
|
👍 from
const cloudevent
to
const cloudEvent |
There was a problem hiding this comment.
In general, I like the idea. Have always really disliked the naming in this module. However, there seems to be a mix of efforts here to both improve things (in a way that breaks backwards compatibility), yet also retains unnecessary functions such as event() (now newEvent()).
I think it's best to either have a clean break and forego backwards compatibility, or be much more cautious about that.
I suggest that we figure out the branching and versioning strategies before we start landing breaking changes.
| @@ -23,7 +23,6 @@ module.exports = { | |||
| BinaryHTTPEmitter, | |||
| BinaryHTTPReceiver, | |||
| HTTPUnmarshaller, | |||
| Cloudevent, | |||
There was a problem hiding this comment.
This is a breaking change to the API. I think we need to branch a v1.x line before doing this, or simply leave this line in for backwards compat.
There was a problem hiding this comment.
Yes, this is a breaking change. I can change the commit message accordingly.
This would be a 2.0, we don't need separate git branches for breaking changes, just major version changes.
There was a problem hiding this comment.
@grant I'm not sure you understand my proposal. I am not suggesting that we have branches for breaking changes.
There was a problem hiding this comment.
OK, let's talk about the breaking change issue separately and reconvene here.
| function event() { | ||
| return new Cloudevent(Spec); | ||
| function newEvent() { | ||
| return new CloudEvent(Spec); | ||
| } |
There was a problem hiding this comment.
If we are exporting the constructor, why even have this function?
There was a problem hiding this comment.
I didn't attempt to change functionality, just the name in this PR.
I don't want functionality change beyond the name changes.
There was a problem hiding this comment.
Seems like an arbitrary distinction. I'd argue that making breaking changes as noted above is a change in functionality.
| function event() { | ||
| return new Cloudevent(Spec); | ||
| function newEvent() { | ||
| return new CloudEvent(Spec); | ||
| } |
There was a problem hiding this comment.
Again - if we are exporting the ctor...
There was a problem hiding this comment.
@lance If I am correct, spec 0.2 had SDK conformance to expose a method event() to create new event instances. For now, it's just to not break the sdk API
There was a problem hiding this comment.
I agree. However, this code was here before. I'm just changing the name.
Signed-off-by: Grant Timmerman <timmerman+devrel@google.com>
grant
force-pushed
the
grant_cloudevent_capitalization
branch
from
fd9acd4
to
74c8b53
Compare
|
I'm still in favor of merging this PR. If we think a part of the PR should be changed, please request it. |
|
Yes - let's land it at some point. Let's just make sure that we know what we are doing with regard to breaking changes before we do that. OK? |
I would like to not say "at some point". I don't understand what the blocker is here as the |
|
@grant Literally 20 minutes ago you said in Slack that you wanted something different with branch management. Is it fine now? |
I'm still understanding the backporting feature. It seems fine in terms of branch management so long as we can make progress with future PRs like this that make breaking changes. |
|
Thanks for your patience and responsiveness @lance. |
Uses the same capitalization for CloudEvents everywhere.
Fixes #100.