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

Minor API improvements #20

Merged
merged 6 commits into from
Jan 10, 2019
Merged

Minor API improvements #20

merged 6 commits into from
Jan 10, 2019

Conversation

dvgica
Copy link
Owner

@dvgica dvgica commented Jan 10, 2019

This one is probably best reviewed commit by commit. It's generally pretty self-explanatory.

Copy link
Contributor

@droustchev droustchev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

// up to whomever made the request.
trait Upstream[-AddressingConfig] {
def metricsTag: String

def addressRequest(request: HttpRequest, addressingConfig: AddressingConfig): HttpRequest

def prepareRequestForDelivery(request: HttpRequest): HttpRequest = request
def requestFilter: RequestFilter[Unit] = NoOpRequestFilter
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we not maintain the verb form here? So filterRequest and filterResponse? Makes the calls in the HttpProxy more readable in my opinion

--- upstream.requestFilter(addressedRequest, ())
+++ upstream.filterRequest(addressedRequest, ())

Copy link
Owner Author

@dvgica dvgica Jan 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The verbs don't apply here, since calling the method does nothing but return the filter. It doesn't apply anything like previously.

In HttpProxy, there's apply implicitly called, it's upstream.requestFilter.apply(addressedRequest, ()).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah ok, I did stumble across the signature here, but I didn't think of the implicit call to apply in the HttpProxy.

@dvgica dvgica merged commit 7d40eb7 into master Jan 10, 2019
@dvgica dvgica deleted the minor-improvements branch January 10, 2019 23:05
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.

2 participants