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 docstrings to system.components.handler #103

Merged
merged 2 commits into from
Jan 15, 2017

Conversation

plexus
Copy link
Contributor

@plexus plexus commented Jan 14, 2017

It took me quite a bit of staring at this code to see what it does exactly, so I figured some docstrings would be nice.

I hope you're open to this kind of contribution. While I'm a big fan of System, I'm finding a lack of documentation for individual components a real stumbling block.

Thanks!

@danielsz
Copy link
Owner

danielsz commented Jan 14, 2017

This is beyond good, Arne. Thank you very much.

I am very much aware that documentation is lacking and was planning to address this in a principled manner, but time is always short. I am very happy about this PR. The Duct components in system were the most pressing ones with regards to documentation.

Please see comments before I am able to merge.

@danielsz
Copy link
Owner

danielsz commented Jan 15, 2017

You nailed it with the explanation. It's perfect.

But in the example code, I think you meant:

:handler (-> (new-handler)
                    (component/using [:endpoint-a :endpoint-b :middleware]))

The framework takes care of combining endpoint-a and (endpoint-b + endpoint-b-middleware), while both get wrapped in the global handler's middleware. Was that your understanding?

@plexus
Copy link
Contributor Author

plexus commented Jan 15, 2017

Ah yes, good catch, I first had an example with only a single endpoint, that's why it ended up like this. I pushed the change.

@danielsz
Copy link
Owner

danielsz commented Jan 15, 2017

Excellent. Thank you, Arne!

There is another thing that needs explaining, it's the fact that you can inject dependencies in middleware.

I wonder if this isn't the right opportunity. On the other hand, it might burden the cognitive load of the reader, and we don't want that.

I'll make an attempt based on your docstring. Here it goes:

  Example:
       (component/system-map
          :db (new-db)
          :endpoint-a (new-endpoint some-routes)
          :endpoint-b (-> (new-endpoint other-routes)
                          (component/using [:endpoint-b-middleware :db]))
          :endpoint-b-middleware (new-middleware {:middleware [wrap-login :component]})
          :middleware (new-middleware {:middleware [,,,]})
          :handler (-> (new-handler)
                       (component/using [:endpoint-a :endpoint-b :middleware]))
          :jetty (-> (new-web-server port)
                     (component/using [:handler])))"

The convention for the middleware component is to pass the keyword :component as argument, and then you can write middleware that wraps the component, like so:

(defn wrap-login [handler {db :db}]
  (fn [request]
    (do-something-with db)
     ...
    (handler request)))))

An endpoint can also receive dependencies other than middleware, so I gave :endpoint-b the :db. ,
I'm not sure if it's necessary to stress that point, though.

@plexus
Copy link
Contributor Author

plexus commented Jan 15, 2017

Yes, I see what you're saying. That's pretty interesting, I actually had a use case for this recently and didn't immediately see how to achieve it.

I would document this in the docstring of new-middleware though, since new-handler really has no knowledge of this. That way the information is a bit more scattered, which is unfortunate, but I think for docstrings it's better to keep them focused, there's already plenty going on in handler as well.

To really show the full picture of how to use endpoint/middleware/handler/server a separate document might be more suitable.

@danielsz danielsz merged commit 4db00f5 into danielsz:master Jan 15, 2017
@danielsz
Copy link
Owner

Yes, that makes sense. Thank you again, Arne, awesome work.

@danielsz
Copy link
Owner

BTW, any tricks to share regarding formatting the doctring?

@danielsz
Copy link
Owner

@plexus, FYI I've added docstrings to the middleware component too.

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