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

Rjf/traversal abstractions clone issue #44

Merged
merged 4 commits into from
Nov 27, 2023

Conversation

robfitzgerald
Copy link
Collaborator

This PR refactors the energy model build process so that all objects in the energy TraversalModelService are wrapped in Arc references if they are large heap objects, so cloning is not expensive. it cleans up the build removing one layer of indirection along the way.

TraversalModelService (and builder) traits were defined in routee-compass but have been moved to routee-compass-core so that they are available from the routee-compass-powertrain crate. build methods now return a TraversalModelError on failure, since CompassConfigurationError is no longer in scope.

It's probably a good low-hanging fruit task to move the rest of the builder traits to the core crate too.

i attempted to update the doc reference from the routee-compass crate that references TraversalModelBuilder, but i'm worried it won't take, that we can't make cross-crate references like that. we may need to replace those builder links with a direct hyperlink to the routee-compass-core docs at docs.rs.

Closes #34.

@nreinicke
Copy link
Collaborator

This makes sense to me and I like moving the builder into the core since it needs to be implemented for any custom traversal model. Perhaps I can merge this into the ndr/phev branch if/when we feel like it's in a good place since there a going to be a couple of merge conflicts related to the changes on the speed grade energy model?

@robfitzgerald
Copy link
Collaborator Author

Perhaps I can merge this into the ndr/phev branch if/when we feel like it's in a good place since there a going to be a couple of merge conflicts related to the changes on the speed grade energy model?

sounds kinda like the same thing as merging this into main now though, right? either way the changes have to get merged into your branch.

Copy link
Collaborator

@nreinicke nreinicke left a comment

Choose a reason for hiding this comment

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

Yeah good point, I'll just merge main into the ndr/phev branch

@robfitzgerald robfitzgerald merged commit 442f9a6 into main Nov 27, 2023
5 checks passed
@robfitzgerald robfitzgerald deleted the rjf/traversal-abstractions-clone-issue branch November 27, 2023 16:57
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.

Energy traversal service gets cloned
2 participants