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

Addressable::Template: Substitution with something that is not a Hash #133

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

levinalex
Copy link

I'm trying to replace my ad-hoc URI-Template parser with Addressable::Template.

However, I don't have the substitutions in Hash form. The current Template implementation assumes that substitutions are Enumerable (it calls inject on mapping)

Not calling inject makes it possible to pass a lambda (or anything else that responds to [] as a substitute:

describe "Substituting from an optional block" do
  sub = lambda { |x| "foo-#{x}" }
  Addressable::Template.new("{bar}/{baz}").expand(sub).to_str.should == "foo-bar/foo-baz"
end

Ideally I'd like to change expand to take a block like so:

template.expand { |field| "foo-#{field}" }

but I'd like to hear what you think about changing the API in that way.

this allows template expansions with lambdas or anything else
that responds to `[]`
@coveralls
Copy link

Coverage Status

Coverage increased (+0%) when pulling 2bdeb5f on levinalex:expand-lambda into 18f2305 on sporkmonger:master.

@sporkmonger
Copy link
Owner

I like this idea, but I'd like to see documentation updated for the methods that would start accepting anything that responds to [] as well as some responds_to? type checking with TypeErrors thrown to keep the exceptions from getting too cryptic. I generally consider NoMethodError a bug in the library.

@sporkmonger
Copy link
Owner

Sorry BTW about the really late reply, somehow the email notification for this PR slipped through the cracks.

@sporkmonger
Copy link
Owner

Also interested in @therabidbanana's take on this PR.

@therabidbanana
Copy link
Collaborator

This seems entirely reasonable to me - documentation would probably be good.

@sporkmonger
Copy link
Owner

@levinalex I like this change, but I'd like to see at least one more test covering the "anything else that responds to []" scenario as well as a documentation update, as @therabidbanana mentioned.

@sporkmonger sporkmonger changed the base branch from master to main July 3, 2021 04:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants