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

Driver registry #68

Merged
merged 2 commits into from May 31, 2016
Merged

Driver registry #68

merged 2 commits into from May 31, 2016

Conversation

trowski
Copy link
Contributor

@trowski trowski commented May 27, 2016

This PR supersedes #64 and addresses the issue raised in #65 (comment).

Instead of being used by Loop, Registry should now be used by classes implementing Driver so each driver object contains state storage. While this is a violation of SRP, I believe this is the best compromise for associating data with a particular loop.

@kelunik
Copy link
Member

kelunik commented May 27, 2016

Should we instead go with an abstract class then instead of an interface?

@bwoebi
Copy link
Member

bwoebi commented May 27, 2016

An abstract class is obviously a possibility, but I think here composition still wins over inheritance.

@bwoebi bwoebi mentioned this pull request May 29, 2016
@AndrewCarterUK
Copy link
Contributor

What is the argument against RegistryInterface (or just Registry) and LoopDriver::getRegistry()?

@bwoebi
Copy link
Member

bwoebi commented May 30, 2016

What is the advantage of this extra layer of access?
In reality it'll just be Loop::getRegistry()->store(); and Loop::getRegistry()->fetch(); instead of Loop::storeState(); and Loop::fetchState(); which are more expressive and shorter.

(And don't argument with SRP here, because it's still a tiny violation as it still provides access to a registry, which is strictly seen a second responsibility)

Also, a trait is implementation wise much easier, it's like one single line and fine.

Thus I see no point in it.

@sagebind
Copy link

sagebind commented May 30, 2016

Too bad we don't have generics 😢. A better solution would be:

interface RegistryState
{
    public static function init(): Self;
}

final class Registry
{
    private array $states;
    public function fetchState<T: RegistryState>(Driver $forDriver): T
    {
        $hash = someHashingFunction(typeName<T>(), $forDriver);
        if (!isset($this->states[$hash]) {
            $this->states[$hash] = T::init();
        }
        return $this->states[$hash];
    }
}

final class Loop
{
    private static Registry $registry;
    public static function fetchState<T: RegistryState>(): T
    {
        return static::$registry::fetchState<T>(static::get());
    }
}


class MyDnsResolver implements RegistryState { /* ... */ }

// Somewhere in code...
Loop::fetchState<MyDnsResolver>()->resolve("google.com");

But alas. We could still do this with ::class and typename strings, but it might not be as elegant.

@bwoebi bwoebi merged commit 1c30c34 into master May 31, 2016
@kelunik kelunik deleted the driver-registry branch May 31, 2016 13:44
trowski added a commit that referenced this pull request May 31, 2016
Conflict due to merge of #68
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.

None yet

5 participants