Apps can hook in their own locator. #15
Conversation
| # end | ||
| def use(locator_name, locator = nil) | ||
| @locator = locator || Base.new(Proc.new) | ||
| end |
There was a problem hiding this comment.
The locators are for a specific app, so gid://foo/Model/id would be located using the block above and gid://bar/Other/id would be located using BarLocator.
def use(app, locator = nil, &locator_block)
raise ArgumentError, 'No locator provided. Pass a block or an object that responds to #locate.' unless locator || locator_block
@locators[app.to_s.downcase] = locator || BlockLocator.new(&locator_block)
end
|
Thanks @kaspth! Commented on the link between |
|
@jeremy I think I got it now. |
| assert_equal options[:returns], GlobalID::Locator.locate(gid) | ||
| ensure | ||
| GlobalID.app = old_app | ||
| end | ||
| end |
|
Cramped build queue? It's been saying waiting for two hours... |
| @@ -3,13 +3,60 @@ module Locator | |||
| class << self | |||
| # Takes either a GlobalID or a string that can be turned into a GlobalID | |||
| def locate(gid) | |||
| GlobalID.find gid | |||
| @locators[GlobalID.app].locate gid | |||
There was a problem hiding this comment.
Rather than the current default app that's used for creating new gids, we need to use the app that's in the URI we're parsing: gid://<app here>/model/id. That means we'll need to parse the gid first and use @locators[gid.app] to do the find.
That suggests that GlobalID#find is what really needs to change. Rather than hardcoding model_class.find(model_id) it needs to ask for the locator for its app, then ask the locator to do the find. The default locator would be an ActiveRecordFinder instance that does def locate(gid) gid.model_class.find gid.model_id end 😁
|
@kaspth you PR need be rebased |
|
Sorry, guys. I'm too sleepy to finish this now. This is as far as I will come today. |
|
❤️ On Mon, Aug 18, 2014 at 1:11 PM, Kasper Timm Hansen <
|
|
@jeremy @seuros I need some advice on what to do with the test 'by GID with only: restriction by module no match' do
found = GlobalID::Locator.locate(@gid, only: Forwardable)
assert_nil found
endThe above test fails because the locator doesn't have the only restriction. What makes the most sense is applying the restriction to all locators. This way users get that behavior for free and we don't complicate the We'd essentially change def locate(gid, options = {})
if gid = GlobalID.parse(gid)
locator_for(gid).locate gid if locate_allowed?(options[:only])
end
end
# global_id.rb
def find(options = {})
Locator.locate(self, options) # just delegate to the app specific locator.
end |
| def parse_encoded_gid(gid) | ||
| new Base64.urlsafe_decode64(repad_gid(gid)) rescue nil | ||
| end | ||
|
|
There was a problem hiding this comment.
^ Merge issue, method is down below private now
|
Looking good! ❤️ |
| # end | ||
| def use(app, locator = nil, &locator_block) | ||
| raise ArgumentError, 'No locator provided. Pass a block or an object that responds to #locate.' unless locator || block_given? | ||
| raise ArgumentError, 'Invalid app name provided. App names with underscores are invalid URIs.' if app.to_s.index('_') |
There was a problem hiding this comment.
👍 for now - when #25 is merged, we'll want to use its app validator for a more thorough check.
|
Looks like this is ready to go @kaspth - could you rebase + squash? |
|
@kaspth Got a test failure on 1.9.3: https://travis-ci.org/rails/globalid/jobs/33109509#L56 |
Move only resctriction to locator. Memoize default locator. Privatize class methods properly. Make app name validating public. Use app validation on GlobalID. Normalize app names when reading or setting locators.
|
@jeremy All done ❤️ Weird test failure. It seems 1.9.3 doesn't like an empty block, so I just inserted some text. |
| end | ||
| end | ||
|
|
||
| test 'app locator is case insensitive' do |
There was a problem hiding this comment.
if gid is not going to be used use _ instead or remove it.
There was a problem hiding this comment.
I thought of it as an extra form of documentation. If people peruse the tests they will know what the argument passed in is.
You still want me to change it?
There was a problem hiding this comment.
Prefer naming the argument that's yielded, too. Omitting the arg is an option. But this is nice and clear for anyone reading the tests to understand how the software behaves.
Apps can hook in their own locator.
Adds #7.