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

Keyword actions #16

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Keyword actions #16

wants to merge 2 commits into from

Conversation

jgaskins
Copy link
Member

@jgaskins jgaskins commented Mar 20, 2018

This PR adds two possibilities for allowing attributes to be specified with keyword args. One adds a separate Action.with_keyword_attributes method and the other determines keyword-ness dynamically during instantiation.

# Can be used as either keyword *or* positional with the same class
CombinedAction = Action.with_attributes(:foo, :bar)
keyword_action = CombinedAction.new(foo: 1, bar: 2)
positional_action = CombinedAction.new(1, 2)

# Only allows keyword arguments
KeywordOnlyAction = Action.with_keyword_attributes(:foo, :bar)
keyword_only_action = KeywordOnlyAction.new(foo: 1, bar: 2)

For the record, I don't think they should both be added, but I think we should choose one based on the following criteria:

  • Is it confusing if we allow both positional and keyword attributes with the same declaration?
  • Is it slower to determine whether to use keywords?
  • How fallible is the keyword/positional distinction in reality?
    • Specifically, I'm thinking of client-side uses here rather than server-side
    • Method arity in dynamically generated methods in Opal isn't always spot-on. I don't know if that's Opal's fault, tbh — it could just be an artifact of it being ambiguous whether to call or apply functions in JS based on the Ruby source.

The separate declaration method is simpler in a micro sense, but is yet another thing to remember. I'd been thinking of deprecating with_attributes entirely and just using create for both to remove distinctions like this, so I'm not super keen on adding another method if we don't need to. But if it clarifies things I'd much rather do that than mess with a bunch of magic.

Not sure if we can do this dynamically because a hash is a valid value
for the first attribute of a hash.
@jgaskins
Copy link
Member Author

I just realized, this complicates currying. For example, given the following action:

CombinedAction = Action.with_attributes(:foo, :bar)
CombinedAction[foo: 1][bar: 2]

Should this be fully applied as CombinedAction.new(foo: 1, bar: 2) or CombinedAction.new({ foo: 1 }, { bar: 2 })? It's obvious in this case, but it's not necessarily obvious in the general case.

If we generated methods entirely differently from the start to specialize on keyword arguments, it would be much more obvious:

KeywordAction = Action.with_keyword_attributes(:foo, :bar)
KeywordAction[foo: 1][bar: 2]

There's no ambiguity here at all. There's no way to misconstrue this as wanting two separate hashes. This isn't implemented at all, but I can add it.

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.

1 participant