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

refactor(database): streamline model merging #185

Merged
merged 1 commit into from
May 22, 2024

Conversation

suchapalaver
Copy link
Contributor

Another small thing I caught while familiarizing myself with the code, this removes the need to create an intermediate vector when merging models in database.

@coveralls
Copy link

coveralls commented May 22, 2024

Pull Request Test Coverage Report for Build 9199775762

Details

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage decreased (-0.04%) to 69.673%

Files with Coverage Reduction New Missed Lines %
common/src/tap/checks/deny_list_check.rs 1 93.64%
Totals Coverage Status
Change from base Build 9195779458: -0.04%
Covered Lines: 3816
Relevant Lines: 5477

💛 - Coveralls

@aasseman
Copy link
Contributor

🤔 I see the streamlining of merging the 2 bits of code in the global model case into 1. But I don't think the early return actually makes the code mode readable

@suchapalaver suchapalaver force-pushed the joseph/reduce-unnecessary-collection branch from 6e9db20 to 0172bce Compare May 22, 2024 21:28
@suchapalaver
Copy link
Contributor Author

suchapalaver commented May 22, 2024

🤔 I see the streamlining of merging the 2 bits of code in the global model case into 1. But I don't think the early return actually makes the code mode readable

NBD, I guess it's personal; the need to mutate the variable in the original threw me when I first read it. But the intention here is to remove the need to collect twice - if it would matter in this case(?).

I've simplified things now, leaving the mut variable alone and using extend instead of collect twice.

@suchapalaver suchapalaver force-pushed the joseph/reduce-unnecessary-collection branch from 0172bce to e3cac7d Compare May 22, 2024 21:57
@aasseman
Copy link
Contributor

Yeah the double collect was a bit nasty

@aasseman
Copy link
Contributor

Well merging this whole thing

models = models
.into_iter()
.map(|model| merge_global(model, &global_model))
.collect();
// Inject a cost model for all deployments that don't have one
models = models
.into_iter()
.chain(
deployments_without_models
.into_iter()
.map(|deployment| CostModel {
deployment: deployment.to_owned(),
model: global_model.model.clone(),
variables: global_model.variables.clone(),
}),
)
.collect();

into 1 was nice though

@suchapalaver suchapalaver force-pushed the joseph/reduce-unnecessary-collection branch from e3cac7d to 786c9fa Compare May 22, 2024 23:33
@suchapalaver
Copy link
Contributor Author

Well merging this whole thing

models = models
.into_iter()
.map(|model| merge_global(model, &global_model))
.collect();
// Inject a cost model for all deployments that don't have one
models = models
.into_iter()
.chain(
deployments_without_models
.into_iter()
.map(|deployment| CostModel {
deployment: deployment.to_owned(),
model: global_model.model.clone(),
variables: global_model.variables.clone(),
}),
)
.collect();

into 1 was nice though

You got it!

@aasseman aasseman merged commit 5ec360b into main May 22, 2024
9 checks passed
@aasseman aasseman deleted the joseph/reduce-unnecessary-collection branch May 22, 2024 23:50
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.

3 participants