Skip to content

v2 - #12

Merged
palkan merged 2 commits into
masterfrom
v2-dev
Aug 20, 2025
Merged

v2#12
palkan merged 2 commits into
masterfrom
v2-dev

Conversation

@palkan

@palkan palkan commented Jun 16, 2025

Copy link
Copy Markdown
Owner

Context

This PR tracks the changes for v2.

What's inside

Checklist:

  • I have added tests
  • I have made corresponding changes to the documentation

Comment thread downstream.gemspec
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"

Copy link
Copy Markdown

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

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Good catch, thanks!

Copy link
Copy Markdown
Owner Author

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

Comment thread lib/downstream/engine.rb
class Engine < ::Rails::Engine
config.downstream = Downstream.config

::GlobalID::Locator.use "downstream" do |gid|

Copy link
Copy Markdown

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.

Comment thread lib/downstream/event.rb
end
end

def define(*fields, &)

Copy link
Copy Markdown

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.

@palkan
palkan marked this pull request as ready for review August 20, 2025 22:18
@palkan
palkan merged commit 488154d into master Aug 20, 2025
6 checks passed
@palkan
palkan deleted the v2-dev branch August 20, 2025 22:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants