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

run-jetty :configurator set to run last #63

Closed
wants to merge 5 commits into from
Closed

run-jetty :configurator set to run last #63

wants to merge 5 commits into from

Conversation

logosity
Copy link

This change allows for replacing the default proxy-handler with a custom handler; the benefit of this (for us) was to allow for logging custom timing and request information from the handler in a way that provides a general solution without adding new configuration options.

The change simply rearranges what's already happening in the run-jetty function to allow the configurator have the final say on what the configuration will be; it does not otherwise change the function's behavior.

@weavejester
Copy link
Collaborator

Seems a reasonable change. Have you tested it?

Also please make the first line of your commit a little shorter. GitHub truncates at 72 characters, and Git tends to recommend a 50 character limit.

Maybe something like: "run-jetty :configurator set last to run"

@logosity
Copy link
Author

we added a test to demonstrate that thread pool can be changed (it and the handler were after the configurator before); otherwise, it's a refactoring (and the other tests pass); we're also using it (though that's not a test really!); I'll edit the first line as well... thanks!

@logosity
Copy link
Author

also added additional tests that the handler set by the configurator is the one on the server object and that there is only one handler set.

@weavejester
Copy link
Collaborator

Could you squash the two test commits together?

@logosity
Copy link
Author

Sure, maybe even squash all three (so the tests are with the code change)?

@weavejester
Copy link
Collaborator

Sure. Having the code in one commit and the tests in the other is also fine, though.

@logosity
Copy link
Author

I'm just going to close this pull request and open another one with the rebased commits...

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

2 participants