Skip to content

Event type#18

Merged
joelhoisko merged 6 commits intomasterfrom
event-type
Sep 3, 2020
Merged

Event type#18
joelhoisko merged 6 commits intomasterfrom
event-type

Conversation

@woksin
Copy link
Copy Markdown
Contributor

@woksin woksin commented Aug 29, 2020

Based of PR #17 Pull that in first

This introduces the new @eventtype decorator, which in fact is just an alias for the @artifact decorator. I think that having the @artifact decorator makes sense. And it makes sense that the registration of Artifacts is done the way that it's done now.

@woksin woksin added the minor label Aug 29, 2020
@woksin woksin requested review from einari, jakhog and joelhoisko August 29, 2020 23:27
@woksin woksin changed the base branch from master to 3.0.0-breaking August 31, 2020 09:51
@woksin woksin changed the base branch from 3.0.0-breaking to master August 31, 2020 09:51
Copy link
Copy Markdown
Contributor

@joelhoisko joelhoisko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, wrapping the old @artifact decorator with @eventType seems fine for me now, in the future we can think if we want to change our terminology and move away from artifact.

@jakhog
Copy link
Copy Markdown
Contributor

jakhog commented Sep 2, 2020

IMO we should not keep the @artifact decorator. Artifact is an abstract type, and it doesn't make sense to instantiate it - so keeping it does not really provide any value. It's ofcourse fine to keep a central store for all the artifacts, but I think the decorator should go.

@joelhoisko
Copy link
Copy Markdown
Contributor

I guess this also affects how much will the user be dealing with the artifact terminology while using the SDK in general (bit off topic for this PR)

Will the user be making ArtifactId's etc? Should the user ever encounter the word artifact or is it something that is strictly internal to only us? What about when the user reads logs about artifacts, can they make the connection between an artifact and what they've been calling event types based on how they use the SDK?

@einari
Copy link
Copy Markdown
Contributor

einari commented Sep 2, 2020

Artifacts is an internal terminology - developers don’t really work with these, they work with more concrete types like event types. From an SDK perspective, it does not add value to the developer having access to artifacts. While internally in the runtime, thats all we know

@woksin
Copy link
Copy Markdown
Contributor Author

woksin commented Sep 2, 2020

That's a good clear up. I'll happily embrace that 😁

@joelhoisko joelhoisko merged commit b23faac into master Sep 3, 2020
@joelhoisko joelhoisko deleted the event-type branch September 3, 2020 08:31
@woksin
Copy link
Copy Markdown
Contributor Author

woksin commented Sep 3, 2020

The changes made to this PR effectively made it a breaking change by removing the @artifact decorator. We should publish a new major version and deprecate these versions (4.1.0) on npm @joelhoisko

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants