-
Notifications
You must be signed in to change notification settings - Fork 3
v2 #12
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
Conversation
| spec.files = Dir.glob("lib/**/*") + %w[LICENSE.txt README.md] | ||
| spec.require_paths = ["lib"] | ||
| spec.required_ruby_version = ">= 2.7" | ||
| spec.required_ruby_version = ">= 3.1" |
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.
Data class is available since 3.2
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.
Good catch, thanks!
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.
Though the library can still be used with older Rubies (without data); we have a note in the Readme
| class Engine < ::Rails::Engine | ||
| config.downstream = Downstream.config | ||
|
|
||
| ::GlobalID::Locator.use "downstream" do |gid| |
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.
Feels like a better place for this code.
| end | ||
| end | ||
|
|
||
| def define(*fields, &) |
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.
attributes and and define feel to me like different strategies to be applied or configured on the Event class.
I do like the new public method, but I think the public interface with this definition doesn't clearly state what's going on, that one variant is mutable and the other immutable. Perhaps the strategy could be defined through an attributes parameter, like immutable: true.
Context
This PR tracks the changes for v2.
What's inside
Downstream::SubscriberclassChecklist: