-
Notifications
You must be signed in to change notification settings - Fork 185
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
Fix #5128 - Properly remove old components/measures when updating #5129
Conversation
9fa472d
to
0285e41
Compare
// Removes all components with uid but NOT currentVersionId | ||
size_t removeOutdatedLocalComponents(const std::string& uid, const std::string& currentVersionId); | ||
|
||
// Removes all measures with uid but NOT currentVersionId | ||
size_t removeOutdatedLocalMeasures(const std::string& uid, const std::string& currentVersionId); |
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.
New LocalBCL methods. Returns the number removed
if (oldComponent && oldComponent->versionId() != newComponent->versionId()) { | ||
LocalBCL::instance().removeComponent(*oldComponent); | ||
} | ||
LocalBCL::instance().removeOutdatedLocalComponents(newComponent->uid(), newComponent->versionId()); |
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.
in updateComponents, remove the outdated local ones if suceeded
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.
The important bit is explained at #5128 (comment)
getComponent does a sqlite3 SELECT, which does NOT guarantee the order of return is in the same order it was inserted. so we can't use it
if (oldMeasure && oldMeasure->versionId() != newMeasure->versionId()) { | ||
LocalBCL::instance().removeMeasure(*oldMeasure); | ||
} | ||
LocalBCL::instance().removeOutdatedLocalMeasures(newMeasure->uid(), newMeasure->versionId()); |
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.
Same on updateMeasures
CI Results for 0285e41:
|
@jmarrec thanks, I like this better |
Pull request overview
Pull Request Author
src/model/test
)src/energyplus/Test
)src/osversion/VersionTranslator.cpp
)Labels:
IDDChange
APIChange
Pull Request - Ready for CI
so that CI builds your PRReview Checklist
This will not be exhaustively relevant to every PR.