-
Notifications
You must be signed in to change notification settings - Fork 192
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
Add missing add_result_measure_info
so that out.osw step result have the same info as before
#5044
Conversation
Found because the OS-resources tests rely on it to locate openstudio_results
bool addResultMeasureInfo(WorkflowStepResult& result, BCLMeasure& measure) { | ||
try { | ||
result.setMeasureType(measure.measureType()); | ||
result.setMeasureName(measure.name()); | ||
result.setMeasureId(measure.uid()); | ||
result.setMeasureVersionId(measure.versionId()); | ||
auto version_modified_ = measure.versionModified(); | ||
if (version_modified_) { | ||
result.setMeasureVersionModified(*version_modified_); | ||
} | ||
result.setMeasureXmlChecksum(measure.xmlChecksum()); | ||
result.setMeasureClassName(measure.className()); | ||
result.setMeasureDisplayName(measure.displayName()); | ||
result.setMeasureTaxonomy(measure.taxonomyTag()); | ||
return true; | ||
} catch (...) { | ||
return false; | ||
} | ||
} |
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.
@@ -84,6 +84,7 @@ void OSWorkflow::applyMeasures(MeasureType measureType, bool energyplus_output_r | |||
LOG(Info, fmt::format("Skipping measure '{}'", measureDirName)); | |||
WorkflowStepResult result = runner.result(); | |||
runner.incrementStep(); | |||
// addResultMeasureInfo(result, bclMeasure); // TODO: Should I really instantiate the BCLMeasure just for this? |
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.
When Skip, I'm not doing it. I think that's fine.
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 test before/after
CI Results for ad85c1a:
|
Pull request overview
Found because the OS-resources tests rely on it to locate openstudio_results
Left is 3.6.1, Right is the C++ CLI:
After: same
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.