-
Notifications
You must be signed in to change notification settings - Fork 222
Fix #5464 - Properly handle "Separator" argument #5471
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
Conversation
…mLogSink
/media/DataExt4/Software/Others/OpenStudio3/src/measure/test/OSMeasure_GTest.cpp:523: Failure
Expected equality of these values:
0
sink.logMessages().size()
Which is: 1
[BOOST_ASSERT] <2> Assertion false failed on line 585 of bool openstudio::measure::OSRunner::validateUserArguments(const std::vector<openstudio::measure::OSArgument>&, const std::map<std::__cxx11::basic_string<char>, openstudio::measure::OSArgument>&) in file /media/DataExt4/Software/Others/OpenStudio3/src/measure/OSRunner.cpp.
cf #5464 (comment) ``` [openstudio.XMLValidator] <1> xsdValidate.parseFileError: XML error 17.1840: Element 'type': [facet 'enumeration'] The value 'Separator' is not an element of the set {'Boolean', 'Double', 'Integer', 'String', 'Choice', 'Path'}. at /home/julien/Downloads/temp/5464/RubyModelMeasure/measure.xml:17 while processing "Separator" [openstudio.XMLValidator] <1> xsdValidate.parseFileError: XML error 17.1840: Element 'type': [facet 'enumeration'] The value 'Separator' is not an element of the set {'Boolean', 'Double', 'Integer', 'String', 'Choice', 'Path'}. ```
| if (script_argument.type().value() == OSArgumentType::Separator) { | ||
| // skip separators | ||
| continue; | ||
| } |
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.
No point validating it.
| arg = OSArgument::makeSeparatorArgument("separator"); | ||
| result.push_back(arg); |
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.
New Test, failing before
| StringStreamLogSink sink; | ||
| sink.setLogLevel(Fatal); | ||
|
|
||
| // call with a good value | ||
| double_arg.setValue(-1.0); | ||
| int_arg.setValue(1.0); | ||
| EXPECT_TRUE(measure.run(model, runner, argumentMap)); | ||
|
|
||
| EXPECT_EQ(0, sink.logMessages().size()) << sink.string(); | ||
| sink.resetStringStream(); |
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.
Fails before
[ RUN ] MeasureFixture.ModelMeasureWithSeparator
/media/DataExt4/Software/Others/OpenStudio3/src/measure/test/OSMeasure_GTest.cpp:523: Failure
Expected equality of these values:
0
sink.logMessages().size()
Which is: 1
[BOOST_ASSERT] <2> Assertion false failed on line 585 of bool openstudio::measure::OSRunner::validateUserArguments(const std::vector<openstudio::measure::OSArgument>&, const std::map<std::__cxx11::basic_string<char>, openstudio::measure::OSArgument>&) in file /media/DataExt4/Software/Others/OpenStudio3/src/measure/OSRunner.cpp.
| <xs:enumeration value="String"/> | ||
| <xs:enumeration value="Choice"/> | ||
| <xs:enumeration value="Path"/> | ||
| <xs:enumeration value="Separator"/> |
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 was missing cf #5464 (comment)
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.
Ah yes, I see there was a second error before:
[openstudio.XMLValidator] <1> xsdValidate.parseFileError: XML error 17.1840: Element 'type': [facet 'enumeration'] The value 'Separator' is not an element of the set {'Boolean', 'Double', 'Integer', 'String', 'Choice', 'Path'}.
|
CI Results for 9adf6d8:
|
shorowit
left a comment
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.
Tested the Windows build and got no errors, LGTM. Thanks @jmarrec!
Pull request overview
Pull Request Author
Labels:
IDDChangeAPIChangePull Request - Ready for CIso that CI builds your PRReview Checklist
This will not be exhaustively relevant to every PR.