-
Notifications
You must be signed in to change notification settings - Fork 99
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
Be able to pass data between Filters #107
Conversation
@@ -281,7 +287,8 @@ mod tests { | |||
.on_downstream_receive(DownstreamContext::new( | |||
vec![], | |||
"127.0.0.1:8080".parse().unwrap(), | |||
vec![9] | |||
vec![9], | |||
HashMap::new(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add a new constructor to the context objects that takes a map for initialization while the current new
constructor initializes with a default empty map? so that we can avoid sweeping changes that add unused default values when we add new fields to structs e.g
DownstreamContext::new(endpoints, from, contents); // Returns a context with no values
DownstreamContext::with_params(endpoints, from, contents, values); // Returns a context with provided values
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the change (or reversion) to ::new
- on review, rather than with_params
I'm wondering if at https://github.com/googleforgames/quilkin/pull/107/files#diff-a539d993fdd469f62785042dc7cb1485R93 we use the From
trait to convert from a Response back to a Context?
I might take it for a spin, see if it works out, if not, I'll switch back to with_params()
src/test_utils.rs
Outdated
match ctx.values.get(key) { | ||
None => { | ||
ctx.values | ||
.insert(key.into(), Box::new("receive".to_string())); | ||
} | ||
Some(value) => { | ||
let mut value = value.downcast_ref::<String>().unwrap().clone(); | ||
value.push_str(":receive"); | ||
ctx.values.insert(key.into(), Box::new(value)); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should be able to use the map entry api to simplify this and avoid re-inserting the value?e.g something like
match ctx.values.get(key) {
Vacant(entry) => entry.insert("receive"),
Occupied(entry) => entry.get_mut().downcast_ref().map(|value| value.push_str("receive"))
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oooh, I knew you would find a more concise way to do this 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done this - working on upper items shortly.
Add a `values` of with a `HashMap` of `Any` to the filter context objects and results, and update the FilterChain to pass that Map between each invocation of a Filter in the chain. Work on #8
28ed781
to
f2ba1b1
Compare
f2ba1b1
to
7bb5c5a
Compare
Updated! Think that looks much neater overall 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Add a
values
of with aHashMap
ofAny
to the filter context objects and results, and update the FilterChain to pass that Map between each invocation of a Filter in the chain.Work on #8