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

Add ConstructorFunc / MiddlewareFunc #32

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

Conversation

jgraettinger
Copy link

@jgraettinger jgraettinger commented Jun 22, 2016

MiddlewareFunc defines a standard signature for a http.HandlerFunc which
accepts a chained http.Handler to invoke. ConstructorFunc supplies an
adapter for converting a MiddlewareFunc into a Constructor. This is most
useful for building middleware with bound parameters via closures.


This change is Reviewable

@jgraettinger
Copy link
Author

Adding a test case is a TODO, but I'd first appreciate confirmation that the idea is acceptable.

@ravisastryk
Copy link

Review status: 0 of 1 files reviewed at latest revision, 1 unresolved discussion.


chain.go, line 33 [r1] (raw file):

      f(w, r, next)
  }
  return http.HandlerFunc(h)

Just a code beautification, avoid 'var h' and use anonymous function call directly. Consider 'return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
f(w, r, next)
})


Comments from Reviewable

MiddlewareFunc defines a standard signature for a http.HandlerFunc which
accepts a chained http.Handler to invoke. ConstructorFunc supplies an
adapter for converting a MiddlewareFunc into a Constructor. This is most
useful for building middleware with bound parameters via closures.
@jgraettinger
Copy link
Author

Apologies for the delay. Reviewing the tests, switching over tagMiddlware appeared as a concise way to add coverage on ConstructorFunc without sacrificing coverage elsewhere, and that's what I've done here. Let me know your thoughts.

PTAL.


Review status: 0 of 2 files reviewed at latest revision, 1 unresolved discussion.


chain.go, line 33 [r1] (raw file):

Previously, ravisastryk wrote…

Just a code beautification, avoid 'var h' and use anonymous function call directly. Consider 'return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
f(w, r, next)
})

Done.

Comments from Reviewable

@jgraettinger
Copy link
Author

Bueller ?

@sb89
Copy link

sb89 commented Nov 17, 2016

+1

1 similar comment
@Akagi201
Copy link

+1

@justinas
Copy link
Owner

Sorry for the delay and I hate to be bikeshedding at this point, but one thing I'd like to do with this is clear separation of this model of middleware as an "adapter".

As the naming stands now, ConstructorFunc seems to be to Constructor as what HandlerFunc is to Handler, which is incorrect. What I'd like to know is:

  • Is there an established name for this middleware pattern? 'Negroni-like'..?
  • What would you think about moving it into a subpackage (.../alice/something)?
  • What would a better name for ConstructorFunc be?

@Akagi201
Copy link

Look at https://godoc.org/github.com/go-web/httpmux#Middleware and https://godoc.org/github.com/go-web/httpmux#MiddlewareFunc. And github search here: https://github.com/search?utf8=%E2%9C%93&q=MiddlewareFunc+language%3Ago&type=Code&ref=searchresults

I think better names are MiddlewareFunc and Middleware, but it wil not be backwards compatibile.

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