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

Rename classes and add more tests #7

Merged
merged 4 commits into from
Nov 23, 2016
Merged

Rename classes and add more tests #7

merged 4 commits into from
Nov 23, 2016

Conversation

shadowhand
Copy link
Contributor

Rename classes by intent and test each class separately.

@shadowhand
Copy link
Contributor Author

@schnittstabil objections to any of this?

Stack is actually a middleware pipe.

DelegateToCallableAdapter is actually a proxy.

Refs #4
Create a common test case and test delegate separately.

Since delegate handles most of the work, most of the testing should
happen there.
All of these are used in the base test case, not in actual tests.
@schnittstabil
Copy link
Contributor

schnittstabil commented Nov 23, 2016

LGTM – I really like the adapter-to-proxy rename.

FWIW, dispatching and pipe management can be separated, because those have different concerns:

$pipe = new MiddlewarePipe($middlewares);
// …
$pipe->append(…);
$pipe->prepend(…);
// …

$dispatcher = new Dispatcher($pipe, $default);
  
$response = $dispatcher->dispatch(ServerRequest::fromGlobals());

– However, maybe, it's a bit too over-engineered…

@schnittstabil
Copy link
Contributor

schnittstabil commented Nov 23, 2016

Hmm, just one further thought: A Dispatcher does not need to know that it have to dispatch MiddlewarePipe instances – it can dispatch arbitrary ServerMiddlewareInterface instances.

Thus, an Equip\Dispatcher library could be useful in several projects.
And an Equip\MiddlewarePipe library may implicitly (or explicitly) require that Equip\Dispatcher

@shadowhand
Copy link
Contributor Author

I don't see a need for it right now, especially if it explicitly depends on MiddlewarePipe.

@shadowhand shadowhand merged commit 51b77a3 into master Nov 23, 2016
@shadowhand shadowhand deleted the fix/renames branch November 23, 2016 13:30
@shadowhand
Copy link
Contributor Author

Released in 0.2.0.

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.

2 participants