-
Notifications
You must be signed in to change notification settings - Fork 209
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
OutputControl:Files #4059
OutputControl:Files #4059
Conversation
Nice work on this @joseph-robertson. Just a note that CI is only set up to run on PRs to develop branch so it won't automatically run on this PR. I kicked off a full build for this branch instead. https://ci.commercialbuildings.dev/blue/organizations/jenkins/openstudio-develop-nightly/detail/openstudio-develop-nightly/685/pipeline |
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.
Nice & polished PR, great work @joseph-robertson
I'll modify the FT/RT tests quickly
The cache-enabled getter in Model is something we should consider, I'm not sure it's really needed or not.
WorkspaceObjectVector idfObjs = w.getObjectsByType(IddObjectType::OutputControl_Files); | ||
ASSERT_EQ(1u, idfObjs.size()); | ||
|
||
WorkspaceObject idf_outputControlFiles(idfObjs[0]); | ||
|
||
EXPECT_EQ("No", idf_outputControlFiles.getString(OutputControl_FilesFields::OutputCSV).get()); | ||
EXPECT_EQ("No", idf_outputControlFiles.getString(OutputControl_FilesFields::OutputMTR).get()); |
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.
I would suggest doing something similar to what I've done for OutputJSON (test all cases where a single one is true, to make absolutely sure we assigned the fields correctly). Using scripting to write the C++ it's not that annoying.
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.
Done
b17aedb
to
a60a3ec
Compare
I rebased onto latest v940-IOFreeze and force-pushed, make sure you refresh your index (git fetch --all) |
If you're comfortable with the tiny changes I've made, go ahead and merge that to v940-IOFreeze! |
Pull request overview
OutputControl:Files
object to SDKPlease read OpenStudio Pull Requests to better understand the OpenStudio Pull Request protocol.
Pull Request Author
Add to this list or remove from it as applicable. This is a simple templated set of guidelines.
src/model/test
)src/energyplus/Test
)src/osversion/VersionTranslator.cpp
)src/openstudio_lib/library/OpenStudioPolicy.xml
)Labels:
IDDChange
APIChange
Pull Request - Ready for CI
so that CI builds your PRReview Checklist
This will not be exhaustively relevant to every PR.