Pass service opts to make-service-method

MichaelBlume commented 8 years ago

Fix #176

weavejester commented 8 years ago

Thanks for the patch. Could you change the commit message so that it's capitalized?

Also I think that we probably want to use a :service option rather than co-opt the :adapter, as there isn't necessarily any overlap between the two.

MichaelBlume commented 8 years ago

Better?

(Should probably be noted that right now, this won't work unless the user explicitly depends on the beta version of ring-servlet)

On Thu, Aug 25, 2016 at 12:42 PM James Reeves notifications@github.com wrote:

Thanks for the patch. Could you change the commit message so that it's capitalized?

Also I think that we probably want to use a :service option rather than co-opt the :adapter, as there isn't necessarily any overlap between the two.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/weavejester/lein-ring/pull/177#issuecomment-242512342, or mute the thread https://github.com/notifications/unsubscribe-auth/AAMv1Wo2AvHDR2firmL-4AbX4BYOU9AOks5qjfAYgaJpZM4Jsstf .

weavejester commented 8 years ago

That'll work, but you added an extra period to the end of the commit message summary line. Can you remove that? So something like:

Pass service options to make-service-method

Fixes #176.

I follow the usual seven rules when writing commit messages. It helps to keep the project history consistent.

MichaelBlume commented 8 years ago

Done =)

On Thu, Aug 25, 2016 at 2:05 PM James Reeves notifications@github.com wrote:

That'll work, but you added an extra period to the end of the commit message. Can you remove that? So something like:

Pass service options to make-service-method

Fixes #176.

I follow the usual seven rules http://chris.beams.io/posts/git-commit/#seven-rules when writing commit messages. It helps to keep the project history consistent.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/weavejester/lein-ring/pull/177#issuecomment-242540324, or mute the thread https://github.com/notifications/unsubscribe-auth/AAMv1T3TZifpsUDAeGu1IVFICs9dEIaJks5qjgOcgaJpZM4Jsstf .

weavejester commented 8 years ago

Thanks! Could you possibly add a small description of the :service key to to the "General Options" section of the README? Maybe something like:

:service - A map of options to be passed to the make-service-method function. This only has an effect when you're deploying your application as a war-file.

Could you also change the commit message to the text I quoted? i.e. "fixes" instead of "fix" since it's describing what the previous line does.

MichaelBlume commented 8 years ago

Done =)

On Thu, Aug 25, 2016 at 7:06 PM James Reeves notifications@github.com wrote:

Thanks! Could you possibly add a small description of the :service key to to the "General Options" section of the README? Maybe something like:

:service - A map of options to be passed to the make-service-method function. This only has an effect when you're deploying your application as a war-file.

Could you also change the commit message to the text I quoted? i.e. "fixes" instead of "fix" since it's describing what the previous line does.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/weavejester/lein-ring/pull/177#issuecomment-242604691, or mute the thread https://github.com/notifications/unsubscribe-auth/AAMv1T8YSV_4SSENwHNfCrZEAz0MrSeMks5qjkoWgaJpZM4Jsstf .

weavejester commented 8 years ago

You don't seem to have committed the changes to the README?

MichaelBlume commented 8 years ago

Ah, sorry, somehow I only saw the second half of your reply.

I'm actually not sure I'm happy with this. An async ring handler will need to be treated as an async handler whether it's started with lein ring service or with installation as a jetty servlet. Shouldn't the user only need to specify this once?

weavejester commented 8 years ago

We could make :async? a top-level option, the same as :port.

MichaelBlume commented 8 years ago

Yeah, makes sense.

On Tue, Aug 30, 2016 at 6:09 PM James Reeves notifications@github.com wrote:

We could make :async? a top-level option, the same as :port.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/weavejester/lein-ring/pull/177#issuecomment-243629540, or mute the thread https://github.com/notifications/unsubscribe-auth/AAMv1SHiyWLq6Zx7IEbp3Xdqkh_b0V9Rks5qlNQqgaJpZM4Jsstf .

weavejester commented 8 years ago

Now that I think about it, that might be the best option. If you want to go ahead and make that change, I think I'd be happy to accept it.

MichaelBlume commented 8 years ago

Given that there's only one service option -- async? -- and we want it to be set at top level, I think I'm going to leave off allowing the user to pass in a map of service options; we may want it later, but there's no good reason for it now.

MichaelBlume commented 8 years ago

weavejester / lein-ring

Pass service opts to make-service-method #177