-
Notifications
You must be signed in to change notification settings - Fork 18
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
feat: reducing context tool #44
Conversation
207590e
to
3da0a55
Compare
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.
Changes required. Mostly documentation and formatting is what I saw in the first pass
res | ||
} | ||
|
||
fn construct_new_payload(req_payload: &Map<String, Value>) -> web::Json<PutReq> { |
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.
Why not take a mutable cloned reference as an argument and skip the first line?
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.
Hey actually, i'll have to do the same where i'm calling also
Like i would have to clone it over there and send it.
Vec::new(); | ||
|
||
for (key, val) in og_overrides.clone() { | ||
match val { |
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 you just do:
match val { | |
val.as_object().unwrap_or(()) |
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.
Hey, I would have to give a default value for unwrap_or, so i went with this
d317257
to
d1840fb
Compare
4c4a52c
to
d09d43c
Compare
d09d43c
to
75d6123
Compare
} | ||
|
||
#[put("/reduce")] | ||
async fn reduce_context( |
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.
@Sauravcv98 is there any specific reason we kept this api under config and not context ?
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.
Hey this actually reduces the entire Config, even the response of this is of type Config
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.
ok make sense ,
can you rename the function to reduce_config ?
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.
Yeah true, was thinking the same
Will rename this to reduce_config
Problem
Too many Context duplicates, where some contexts are a subContext of other one's with the same overrides, so the parent one is not even required
Solution
Removing contexts if we find any one subcontext with the same override key, then we remove the override key from the parent context, now if all of the override keys are removed from the parent context then we can remove the entire context, so we just delete it.