search

ibpsa / project1-boptest

Building Optimization Performance Tests
Other
109 stars 70 forks source link

Issue73 messaging #441

Closed dhblum closed 2 years ago

dhblum commented 2 years ago

For #73.

dhblum commented 2 years ago

@SenHuang19 Tests pass. Want to take a look and give it another review? I think we're pretty much there.

dhblum commented 2 years ago

@JavierArroyoBastida @kbenne. This is a PR for the API changes that introduce more structure and opportunity to give messages and errors. Take a look and see if you have any comments?

SenHuang19 commented 2 years ago

@dhblum Very nice work, just a few small comments:

  1. https://github.com/ibpsa/project1-boptest/blob/issue73_messaging/testcase.py#L267 to #272 seems to be not necessary.
  2. https://github.com/ibpsa/project1-boptest/blob/issue73_messaging/restapi.py#L92: do we need "required=True"?
  3. For the javascript and julia code, we could create a function to avoid referring "payload" directly multiple times
dhblum commented 2 years ago

@SenHuang19 Thanks for the quick review.

  1. L267 - L272 is necessary to ensure the _activate value is either 1 or 0, and nothing else.
  2. Yes, added in https://github.com/ibpsa/project1-boptest/commit/19700b553c7cf97377adfbb275fd9922e6cd9e38.
  3. We could and I understand the limit on code replication, but I'm not that proficient in those languages to do that and I think this is pretty straightforward for now for users to see without too many layers of code. Do you have a strong feeling about it?
dhblum commented 2 years ago

Thanks @JavierArroyoBastida. I've addressed your comments in appropriate commits, thanks for pointing out those oversights. I've also replied to your final comment which is a little more complicated. Let's continue the discussion there.

dhblum commented 2 years ago

@SenHuang19 and @JavierArroyoBastida. I added a few more commits since you may have reviewed, but all in all, I think this is ready to merge once tests pass and would like to in the next few days. Let me know if you disagree. @SenHuang19 let me know also if you have any more comments on the comment here.