Skip to content
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

Issue1059 control io no socket #1170

Merged
merged 37 commits into from
Aug 13, 2019
Merged

Issue1059 control io no socket #1170

merged 37 commits into from
Aug 13, 2019

Conversation

dhblum
Copy link
Contributor

@dhblum dhblum commented Jul 11, 2019

For #1059.

@dhblum dhblum requested a review from mwetter July 11, 2019 23:58
@dhblum
Copy link
Contributor Author

dhblum commented Jul 12, 2019

FYI @JavierArroyoBastida, I've made this PR and asked for a review by @mwetter .

Copy link
Contributor

@mwetter mwetter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dhblum : This is in my view fine to merge. I reviewed all code and documentation and made changes to the documentation where needed. I also added info for all package.mo files.

Note the change in 35ccf6e in which I changed the parameter name to lower case for consistency (instance names are typically lower case). This probably has side effects in downstream tools. I am not sure how disruptive this will be.

If you agree, please ask @PMehrfeld for a review so that we have a non-LBL and non-KU Leuven person reviewing it before it can be merged.

@dhblum
Copy link
Contributor Author

dhblum commented Jul 18, 2019

@mwetter Thank you for completing your review. I agree with all of your suggestions, except made one correction to revision documentation, which I've fixed in efd933f.

The change in parameter name to lower case is noted. It will affect downstream tools, but should be fixed easily. I've already added it as a to-do in the BOTEST repo at ibpsa/project1-boptest#49.

@dhblum dhblum requested a review from PMehrfeld July 18, 2019 18:36
@mwetter
Copy link
Contributor

mwetter commented Jul 18, 2019

Thanks @dhblum .
@PMehrfeld , this is ready for review and merging.

@mwetter
Copy link
Contributor

mwetter commented Aug 13, 2019

@PMehrfeld : can you please review and merge if you agree with the implementation.

Copy link
Contributor

@PMehrfeld PMehrfeld left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mwetter : Sorry, I must have overlooked this review request unintentionally!

@PMehrfeld PMehrfeld merged commit a33c622 into master Aug 13, 2019
@PMehrfeld PMehrfeld deleted the issue1059_controlIONoSocket branch August 13, 2019 08:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants