-
Notifications
You must be signed in to change notification settings - Fork 397
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 #10718 - DXCoils StandardRatings OutputReportPredefined::addFootNoteSubTable produces invalid XML #10860
base: develop
Are you sure you want to change the base?
Conversation
7ac4834
to
5374b1b
Compare
…ding numbers and using and  
@jmarrec it has been 7 days since this pull request was last updated. |
@jmarrec it has been 8 days since this pull request was last updated. |
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.
This is an interesting set of changes. Ultimately I guess you are just collecting a few repeated strings into one location (great!!) and then doing some work to make sure it is valid output. You went above and beyond to bring the pytest work to the core CMakeLists scope and built some great tests to exercise your changes. I'm very happy with this. Any thoughts @JasonGlazer ?
I'm pushing a fix to the license year for the 2 new python files, but this appears ready anytime unless anyone has comments or objections. |
The footnotes now have HTML in them. I wonder if they work with plain text tables and CSV tables? (or did they never work for those?) |
Take DXCoilSystemAuto.idf, modify the OutputControl:Table:Style diff --git a/testfiles/DXCoilSystemAuto.idf b/testfiles/DXCoilSystemAuto.idf
index 398ea60ae8..94b4da3978 100644
--- a/testfiles/DXCoilSystemAuto.idf
+++ b/testfiles/DXCoilSystemAuto.idf
@@ -1859,7 +1859,7 @@
Output:Constructions,Constructions;
OutputControl:Table:Style,
- HTML; !- Column Separator
+ All; !- Column Separator
Output:Table:SummaryReports,
AllSummary; !- Report 1 Name and here it is, with develop |
Pull request overview
Pull Request Author
Add to this list or remove from it as applicable. This is a simple templated set of guidelines.
Reviewer
This will not be exhaustively relevant to every PR.