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

Allow access for attributes with reserved names via #[] (fixes #56) #66

Merged
merged 1 commit into from
Jan 23, 2018

Conversation

flash-gordon
Copy link
Member

No description provided.

@flash-gordon flash-gordon merged commit 7fc641e into master Jan 23, 2018
@flash-gordon flash-gordon deleted the protected-names branch January 23, 2018 18:15
@attributes[name]
else
public_send(name)
end
Copy link
Member

Choose a reason for hiding this comment

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

oh man, array + attribute lookup on each attr value read sounds not too good, perf-wise :/ Another thing is that now I can also see that public_send here was a mistake from the very beginning. I think it should always just do @attributes[name] or even @attributes.fetch(name)

Copy link
Member Author

Choose a reason for hiding this comment

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

@solnic it's a set, which internally is implemented via a hash

Copy link
Member

Choose a reason for hiding this comment

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

@flash-gordon wdyt about leaving it as @attributes[name]? assuming we're OK with hash-like behavior, this would make perfect sense. I'm a bit "meh" when it comes to returning nil for attribute names that are not valid though...

Copy link
Member Author

Choose a reason for hiding this comment

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

@solnic that's exactly the point. At the moment, there is no difference between calling methods and using #[], tbh I get used to this behavior but don't depend on it. After all, we can raise an error on accessing missing keys, it's not like hash behaves but does anyone care?

Copy link
Member

Choose a reason for hiding this comment

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

@flash-gordon right, so let's try @attributes.fetch(name), rescue KeyError and raise UnknownAttributeError or suttin. Sounds good?

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 will do

Copy link

Choose a reason for hiding this comment

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

Turns out, I was relying on this behavior for a project of mine. I'm using Dry::Struct to write a presenter library, and I was using it with Rails to generate urls. I had a superclass like this:

class Presenter < Dry::Struct
  transform_types { |t| t.meta(omittable: true) }
  attribute :url, Types::Url

  def url
    url_for(self)
  end
end

Then, subclasses could either override that method themselves, or when one presenter is calling another it could be passed in if needed, in case of nested routes.

class WidgetCollection < Presenter
  attribute :user_id, Types::Integer
  attribute :widgets, Types::Array

  def url
    user_widgets_path(user_id)
  end

  def widgets
    @widgets.map { |widget| WidgetPresenter.new(widget: widget, url: widget_path(user_id: user_id, widget) }
  end
end

Because it now uses #fetch instead of #public_send, I can no longer access "attributes" that are overridden by explicit methods. I also cannot use Types::URL.default { } because that block is eval'd in the class context rather than the instance (undefined method 'url_for' for Presenter:Class).

Any chance of rolling back this change? Should I open an issue? I can also just redefine the #[] method in my Presenter back to the old behavior.

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.

3 participants