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

feat: reducing context tool #44

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

feat: reducing context tool #44

wants to merge 4 commits into from

Conversation

Sauravcv98
Copy link
Collaborator

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.

@Sauravcv98 Sauravcv98 requested a review from a team as a code owner May 10, 2024 07:32
@Sauravcv98 Sauravcv98 force-pushed the Reduce-context-tool branch 3 times, most recently from 207590e to 3da0a55 Compare May 10, 2024 09:41
Copy link
Collaborator

@Datron Datron left a 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

crates/context_aware_config/src/api/config/handlers.rs Outdated Show resolved Hide resolved
res
}

fn construct_new_payload(req_payload: &Map<String, Value>) -> web::Json<PutReq> {
Copy link
Collaborator

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?

Copy link
Collaborator Author

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.

crates/context_aware_config/src/api/config/handlers.rs Outdated Show resolved Hide resolved
crates/context_aware_config/src/api/config/handlers.rs Outdated Show resolved Hide resolved
Vec::new();

for (key, val) in og_overrides.clone() {
match val {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you just do:

Suggested change
match val {
val.as_object().unwrap_or(())

Copy link
Collaborator Author

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

crates/context_aware_config/src/api/config/handlers.rs Outdated Show resolved Hide resolved
crates/context_aware_config/src/api/config/handlers.rs Outdated Show resolved Hide resolved
crates/context_aware_config/src/api/config/handlers.rs Outdated Show resolved Hide resolved
@knutties knutties changed the title Reducing context tool feat: reducing context tool May 14, 2024
@Sauravcv98 Sauravcv98 force-pushed the Reduce-context-tool branch 2 times, most recently from d317257 to d1840fb Compare May 20, 2024 10:23
@Sauravcv98 Sauravcv98 force-pushed the Reduce-context-tool branch 3 times, most recently from 4c4a52c to d09d43c Compare May 21, 2024 06:13
}

#[put("/reduce")]
async fn reduce_context(
Copy link
Collaborator

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 ?

Copy link
Collaborator Author

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

Copy link
Collaborator

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 ?

Copy link
Collaborator Author

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

@Datron Datron added the priority Things that need immediate attention label May 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority Things that need immediate attention
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants