Skip to content
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

Always expect an event to be passed to subscribers. #40

Closed
wants to merge 1 commit into from

Conversation

pat
Copy link

@pat pat commented Mar 9, 2021

From what I can understand, the previous behaviour passed the event through - but, because of Dry::Events::Event#[], it could be implicitly converted into keyword arguments.

With Ruby 3 no longer making implicit conversions of keyword arguments, I don't see this old behaviour as feasible - instead, the built-in subscribers should instead expect the event object, and get the particular keyed values out from it.

I'm not sure how significant a change this is, but I can foresee it breaking usage for others if they're using keyword arguments in their subscribers. I also note that there's been detailed discussions around payloads in #32 - perhaps this PR is moot depending on how that plays out? 🤷🏻‍♂️

From what I can understand, the previous behaviour passed the event through - but, because of `Dry::Events::Event#[]`, it could be implicitly converted into keyword arguments.

With Ruby 3 no longer making implicit conversions of keyword arguments, I don't see this old behaviour as feasible - instead, the built-in subscribers should instead expect the event object, and get the particular keyed values out from it.
@pat pat requested a review from solnic as a code owner March 9, 2021 05:38
@solnic
Copy link
Member

solnic commented Mar 9, 2021

Thanks. This is definitely something that should be addressed before 1.0.0 release. I'll keep this open for the time being because it's a breaking change that should be coordinated with other people.

@solnic solnic added the on hold label Mar 9, 2021
@solnic solnic added this to the 1.0.0 milestone Mar 9, 2021
@solnic
Copy link
Member

solnic commented Jul 1, 2022

Looks like this was addressed in main via ce652f2. Closing this one.

@solnic solnic closed this Jul 1, 2022
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.

2 participants