Add native support for measure argument methods in OsLib_HelperMethods #5142
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…S >= 10.14 supports it, and E+ is 10.15 minimum anyways
…static as it should
* Try to align getPastStepValuesForName on check_upstream_measure_for_arg in what it picks for measure name * getPastStepValuesForMeasure: allow matching on either the WorkflowStep's measureDirName or name, or the WorkflowStepResult's measureName * Scope the test blocks for readability and avoiding side effects (less chances of making a mistake)
…rn defaultValue if value not set
jmarrec
added
the
Pull Request - Ready for CI
jmarrec
commented
Apr 11, 2024
Comment on lines
+789
to
+794
| // This function aims to replace OsLib_HelperMethods.createRunVariables | ||
| // TODO: should this call validateUserArguments like the ruby counterpart? At least it validates that the defaults are supposed to match etc | ||
| if (!validateUserArguments(script_arguments, user_arguments)) { | ||
| registerError("Invalid argument values."); | ||
| LOG_AND_THROW("Invalid argument values."); | ||
| } |
There was a problem hiding this comment.
TODO: do we want this? I think we do.
Comment on lines
+802
to
+806
| if (arg.hasValue()) { | ||
| argument_values[name] = arg.valueAsJSON(); | ||
| } else if (arg.hasDefaultValue()) { | ||
| // TODO: is this wanted? | ||
| argument_values[name] = arg.defaultValueAsJSON(); |
There was a problem hiding this comment.
TODO: do we include defaultValue if not value?
Comment on lines
+820
to
+830
| // TODO: should we match on any of these three? | ||
| if (istringEqual(measureName, step.measureDirName())) { // The directory name, eg `IncreaseWallRValue` | ||
| LOG(Trace, "Step matches on measureDirName"); | ||
| } else if (auto s_ = step.name(); istringEqual(measureName, *s_)) { // An optional, abritrary one | ||
| LOG(Trace, "Step matches on name"); | ||
| } else if (auto s_ = stepResult_->measureName(); | ||
| istringEqual(measureName, *s_)) { // The xml one, eg `increase_insulation_r_value_for_exterior_walls_by_percentage` | ||
| LOG(Trace, "Step matches on Step Result's measureName"); | ||
| } else { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Right now it matches on any. Maybe we shouldn't, maybe we should have three functions.
Maybe we return an array of hashes instead, so user can pick.
| } | ||
| } | ||
| break; |
There was a problem hiding this comment.
Breaking at first one found right now. Maybe that's bad.
There was a problem hiding this comment.
Also, I don't think this method was in the OSLibHelpers @joseph-robertson . It could be useful nonetheless
Comment on lines
+852
to
+858
| std::string measure_name = step.measureDirName(); // The directory name, eg `IncreaseWallRValue` | ||
| if (auto s_ = step.name()) { // An optional, abritrary one | ||
| measure_name = std::move(*s_); | ||
| } | ||
| // if (auto s_ = stepResult_->measureName()) { // The xml one, eg `increase_insulation_r_value_for_exterior_walls_by_percentage` | ||
| // measure_name = std::move(*s_); | ||
| // } |
There was a problem hiding this comment.
Trying to do it like check_upstream_measure_for_arg
Comment on lines
+863
to
+865
| if (auto value = root["value"]) { | ||
| step_values[measure_name] = value; | ||
| // TODO: check_upstream_measure_for_arg was breaking early, and returning a {:value => value, :measure_name => measure_name} format |
joseph-robertson
approved these changes
Apr 11, 2024
There was a problem hiding this comment.
I approve, but I defer to @DavidGoldwasser for whether all expected functionality is here.
…true * Move logic to Variant::isTruish to determine if a variant is trueish (whether that's bool true, str = "true", int != 0, double != 0.0 * Add a OSRunner::prepareForMeasureRun() that takes no arg, protected, and friend OSWorkflow so we can use it without the penatly of instantiating an OSMeasure * In Apply Measure, when __SKIP__, correctly call prepareForMEasureRun before incrementing the step. And do load the BCLMeasure to write the resultInfo like the workflow-gem was doing
|
CI Results for c6ff09c:
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Pull request overview
Pull Request Author
src/model/test)src/energyplus/Test)src/osversion/VersionTranslator.cpp)Labels:
IDDChangeAPIChangePull Request - Ready for CIso that CI builds your PRReview Checklist
This will not be exhaustively relevant to every PR.