Per-endpoint configuration #109
Conversation
There was a problem hiding this comment.
This is awesome! I have left a few questions asking for some more clarification, but you basically answered all the questions I had regarding how to go about sharing this state across this App. I can't stress on the fact that PR taught me a bunch of new things around the Rust type system :)
I can already see how to integrate the logger into this.
|
@bIgBV Go ahead! It would be really helpful if I can get some feedback about the API design. |
* This is because the Confiugration type is a App global sharable TypeMap which is avaiable across both middleware and endpoints. Therefore, this can be used to store arbitrary types and not only configuartion. * Tis commit renames all occurrances of Configuration to Store and updates the parameters and struct field names appropriately.
* Have simple hook into the App by the way of the `setup_configuration` method. This is pretty basic rightn now, but can be the point of entry for hooking in custom configuration.
src/configuration.rs
Outdated
| //! Types for managing and extracting configuration. | ||
|
|
||
| use std::any::{Any, TypeId}; | ||
| use std::collections::HashMap; |
There was a problem hiding this comment.
A possible performance improvement here would be to use the hashbrown crate.
There was a problem hiding this comment.
Isn't that currently being evaluated to be used as the default HashMap implementation in the std? If that's the case then do you think it's a good idea to replace it right now? (Though to be fair, it might be at least a few releases away..)
There was a problem hiding this comment.
I heard that hashbrown is used only in rustc, not libstd.
There was a problem hiding this comment.
Oh I missed this. But yeah, it's landing in libstd; people made a PR during RustFest Rome for this!
There was a problem hiding this comment.
This looks really good! It's very thorough, and I think we might be able to merge this as-is. Keen to try this out!
edit: also wanted to mention I definitely learned some new things reading this code. Thanks for putting in the work @tirr-c!
|
There is a PR targeting my branch, tirr-c#1, that @bIgBV have worked on renaming things and adding app configuration as a configuration item. I've approved the PR but I haven't merged that since I was busy. I'll merge it and add some docs. Can you please review this again then, @yoshuawuyts? |
|
@tirr-c done! -- I think that patch looks pretty good! Don't have any blockers; green light from me! ✨ |
Default configuration
|
The build is failing on a couple of fairly trivial clippy lints, I can make the changes, but I'm not sure if I can and/or which branch to change. |
|
@bIgBV No worries, I'll handle those! |
|
Builds are failing because nightly can't be installed rust-lang/rust#57488. I think we should be good to merge once we can get tests to pass! |
|
I love this but I have some issues with it that I can't quite put my finger on exactly yet. I guess one issue is the naming of some things e.g. |
|
So looks like travis was failing on installing nightly in the container. I just restarted the build and looks like whatever issue was occurring a couple of days ago is now solved. Are there any other blockers for this PR as I want to get another PR open for a default config file and the related supporting code open. Plus once the discussion in #8 reaches some consensus, I'm sure this will help a lot for that as well. EDIT: Looks like I spoke too soon :( Looks like the build is failing on formatting a dependency. Is that expected? |
|
Travis is passing again, and from the comments there seem to be no blockers. I propose we merge! |
Description
This PR adds per-endpoint configuration mechanism.
Each configuration item is represented as a value of its own type. Items can be applied to endpoints and (sub)routers, and child configuration inherits from that of parent. The point when
configis called does not matter; when the router setup ends, all configuration items of the router are applied to the children (except overridden).The first three commits implements the feature with
serdeandtoml. The other commits use a minimal, custom typemap for configuration.Motivation and Context
Resolves #5.
Currently we don't provide any way to configure middleware, extractors etc. Using configuration, we can adjust various aspects of how components of Tide work, like limiting request body size.
How Has This Been Tested?
examples/configuration.rs). Tested with HTTPie and works as expected.Configurationitself and some forRouter.Types of changes
Checklist: