Update fmt check script, cleanup a few issues#11311
Merged
Conversation
mitchute
commented
Oct 31, 2025
| for (int i = 1; i <= nReportPeriods; i++) { | ||
| std::string ReportPeriod_Resilience_Summary = fmt::format( | ||
| "{} Resilience Summary for Reporting Period {}: {}", kw, i, ReportPeriodInputData(i).title, ReportPeriodInputData(i).totalElectricityUse); | ||
| "{} Resilience Summary for Reporting Period {}: {} {}", kw, i, ReportPeriodInputData(i).title, ReportPeriodInputData(i).totalElectricityUse); |
Collaborator
Author
There was a problem hiding this comment.
Found this issue. We might want to add a bit more here.
Comment on lines
-6526
to
+6527
| format("...Reheat coil outlet node name = {}" + state.dataLoopNodes->NodeID(SupHeatCoilOutletNode))); | ||
| ShowContinueError(state, format("...UnitarySystem outlet node name = {}" + state.dataLoopNodes->NodeID(this->AirOutNode))); | ||
| format("...Reheat coil outlet node name = {}", state.dataLoopNodes->NodeID(SupHeatCoilOutletNode))); | ||
| ShowContinueError(state, format("...UnitarySystem outlet node name = {}", state.dataLoopNodes->NodeID(this->AirOutNode))); |
Collaborator
Author
There was a problem hiding this comment.
I think this concatenates the strings instead of replacing the argument in the placeholder. Updated here.
Comment on lines
+391
to
+429
| class TestCheckFormatStrings(unittest.TestCase): | ||
| PASS = 0 | ||
| FAIL = 1 | ||
|
|
||
| def test_valid_cases(self): | ||
| test_options = [ | ||
| 'format("{}", arg1)', | ||
| 'format("{}{}=\"{}\"", RoutineName, s_ipsc->cCurrentModuleObject, s_ipsc->cAlphaArgs(1))', | ||
| 'fmt::format("PLR = {:7." + std::to_string(DecimalPrecision) + "F}", fmt::join(PLRArray, ","))', | ||
| 'format("...{} is < 2 {{C}}. Freezing could occur.", cNumericFields(17))', | ||
| 'format(R"({}="{}" invalid {}="{}" not found.)",\n CurrentModuleObject,\n ventSlab.Name,\n cAlphaFields(4),\n state.dataIPShortCut->cAlphaArgs(4))', | ||
| 'format("{}{}{}{}{}{}", "Occurs for Node=", NodeName, ", ObjectType=", ObjectType, ", ObjectName=", ObjectName)', | ||
| 'format("{}{}", RoutineName, "Node registered for both Parent and \"not\" Parent")' | ||
| 'format("{}\n", EnergyPlus::Constant::unitNames[(int)meter->units])' | ||
| ] | ||
|
|
||
| tmp_dir = Path(tempfile.mkdtemp()) | ||
| tmp_file = tmp_dir / "valid.cc" | ||
|
|
||
| with open(tmp_file, 'w') as f: | ||
| for line in test_options: | ||
| f.write(line) | ||
|
|
||
| self.assertEqual(self.PASS, check_format_statements([tmp_file])) | ||
|
|
||
| def test_invalid_cases(self): | ||
| test_options = [ | ||
| 'format("{}", arg1, arg2)', | ||
| 'format("{}{}=\"{}\"")', | ||
| ] | ||
|
|
||
| tmp_dir = Path(tempfile.mkdtemp()) | ||
| tmp_file = tmp_dir / "invalid.cc" | ||
|
|
||
| with open(tmp_file, 'w') as f: | ||
| for line in test_options: | ||
| f.write(line) | ||
|
|
||
| self.assertEqual(self.FAIL, check_format_statements([tmp_file])) |
Collaborator
Author
There was a problem hiding this comment.
There's a bunch of parsing voodoo in this script. These tests seem to be enough to capture most of our format uses.
mitchute
commented
Oct 31, 2025
Comment on lines
+3386
to
+3390
| std::string ReportPeriod_Resilience_Summary = fmt::format("{} Resilience Summary for Reporting Period {}: {} {}", | ||
| kw, | ||
| i, | ||
| ReportPeriodInputData(i).title, | ||
| ReportPeriodInputData(i).totalElectricityUse); |
Collaborator
Author
There was a problem hiding this comment.
I ran regressions locally without issue, and CI similarly is not reporting any diffs here, which I assume means these lines are not being hit. Anyone care to comment on whether more should be added here?
Contributor
There was a problem hiding this comment.
I think the resilience reports only run for annual simulations?
Collaborator
Author
There was a problem hiding this comment.
Maybe. Just ran regressions forcing annual simulations on the following and there were no diffs.
1ZoneUncontrolled_Win_ResilienceReports_ReportingPeriod_AllTable.idf
1ZoneUncontrolled_Win_ASH55_Thermal_Comfort.idf
1ZoneEvapCooler_ReportPeriodPowerOutage.idf
1ZoneEvapCooler_ReportPeriodPowerOutage_naturalVent.idf
1ZoneUncontrolled_Win_ResilienceReports_ReportingPeriod.idf
1ZoneUncontrolled_Win_ResilienceReports.idf
Collaborator
Author
|
Good enough for now on this, but we may need a follow-up issue on the resiliency tables. |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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
While reviewing #11165, I noticed the code integrity check was failing due to a false positive on a new
formatstring. This PR updates the script to handle that new case and fixes a couple issues found during the process.Pull Request Author
Reviewer