Coil API Refactor (Part II -- simplify ReportCoilSelection)#11517
Coil API Refactor (Part II -- simplify ReportCoilSelection)#11517
Conversation
amirroth
added
Refactoring
|
|
|
|
amirroth
left a comment
amirroth
left a comment
There was a problem hiding this comment.
Maybe the simplest PR I've ever had. Certainly top five.
| if (this->isCoilReportObject) { | ||
| state.dataRptCoilSelection->coilSelectionReportObj->setCoilEntAirTemp( | ||
| state, this->compName, this->compType, this->autoSizedValue, this->curSysNum, this->curZoneEqNum); | ||
| ReportCoilSelection::setCoilEntAirTemp( |
There was a problem hiding this comment.
This is basically the only type of change in the entire PR. A lot of search-replace and letting the compiler flag the remaining changes that need to be performed by hand.
| #endif // GET_OUT | ||
| }; | ||
|
|
||
| #ifdef GET_OUT |
There was a problem hiding this comment.
Eliminated these while I was at it.
|
|
||
| extern const std::array<bool, (int)CoilType::Num> coilTypeIsHeatPump; | ||
|
|
||
| #ifdef GET_OUT |
| HVAC::CoilType heatCoilType = HVAC::CoilType::Invalid; | ||
|
|
||
| HVAC::CoilType coilType = HVAC::CoilType::Invalid; | ||
| int coilReportNum = -1; |
There was a problem hiding this comment.
Every coil now has a coilReportNum variable which is assigned when the coil is first created.
| heatingCoil.heatCoilType = HVAC::CoilType::HeatingElectric; | ||
| heatingCoil.coilType = HVAC::CoilType::HeatingElectric; | ||
|
|
||
| heatingCoil.coilReportNum = ReportCoilSelection::getReportIndex(state, heatingCoil.Name, heatingCoil.coilType); |
There was a problem hiding this comment.
Create coil, assign coilReportNum.
| struct ReportCoilSelectionData : BaseGlobalStruct | ||
| { | ||
|
|
||
| std::unique_ptr<ReportCoilSelection> coilSelectionReportObj; |
There was a problem hiding this comment.
There is no need for this embedded object just to make the module interface "object-oriented". Object-orientation is in quotes because it wasn't even being applied to the correct object, it was being applied to the container object rather than the individual coilReport objects, i.e., coilContainerObject->setCoilData(coilNum, data); instead of coilReportObject->setCoilData(data);. Anyways, I made this into a simple procedural API.
Is something still wrong with the Ubuntu runner? I don't understand how this type of PR can generate these types of diffs. |
|
|
|
|
I had a theory this morning, so I reran regressions a couple times. But that theory didn't pan out. I'll keep hunting. The latest rerun + local testing is showing that some of the table diffs are real. |
I'm more inclined to believe the MacOS diffs (which have two kinds of diffs including table big diffs) in 14 files than the Ubuntu diffs (which have 16 kinds of diffs in 400 files). |
|
Late edit:
The latest Ubuntu regressions show this as well. |
|
|
Thanks. I will focus on those. |
These diffs are due to coils appearing in the tables in different orders. I thought that Edwin had modified the table comparison script to handle this case, but maybe not. Or maybe it is not applied to all tables. |
Let me look into the what the regression tool is doing. I also thought that. I just rans some tests locally and I do see that in the coil sizing summary table, the Coil Type column entries have changed from title case to all caps. I doubt that's the problems, but I'll check. |
|
|
| std::string tmpZoneList; | ||
| for (const auto &vecLoop : c->zoneName) { | ||
| tmpZoneList += vecLoop + "; "; | ||
| state, state.dataOutRptPredefined->pdchCoilType, c->coilName_, HVAC::coilTypeNamesUC[(int)c->coilType]); |
There was a problem hiding this comment.
The capitalization "issue" is here.
|
|
|
I reworked the regression tool to do a better job reporting table diffs and handling cases where the rows have been reordered. That has been deployed, and I've reran the workflow on the latest commit. The regression results posted here now are consistent with what we discussed above. |
|
|
|
|
So, the existing capitalization behavior is inconsistent. Most coil types are reported in TitleCase but a few (mostly heating coils from the looks of it) are reported UPPERCASE. In this branch, all coil types are reported in TitleCase. I think that is preferred even if it is technically a diff. |
I agree. I did a few quick spot check, and it all looks OK. I deleted and restarted that failing test. It should report back shortly and we can merge this. |
|
All good here. Merging. |
3 participants
Second part of the Coil API refactor. This part simplifies ReportCoilSelection which was unnecessarily "object-oriented".