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

Issue704 update bestest hydronic heat pump manufacturer documentation #714

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Jaap-Neven
Copy link
Contributor

Adapted the link in the index.html file constructing the github pages website to refer to the correct datasheet as found here. Previous link pointed to the main website of Carrier (in Greek).

@dhblum dhblum self-requested a review December 4, 2024 15:49
Copy link
Collaborator

@dhblum dhblum left a comment

Choose a reason for hiding this comment

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

Thanks @Jaap-Neven for the PR. Can you:

  1. Remove documentation files which I think are related to Issue582 update testcase1 and testcase 3 documentation #706.
  2. Also update link in the model html documentation, I think it would here: https://github.com/Jaap-Neven/project1-boptest/blob/issue704_bestest_hydronic_HP_manufacturer_documentation/testcases/bestest_hydronic_heat_pump/models/BESTESTHydronicHeatPump/TestCase.mo#L580
  3. Add note of the update in the releasenotes.md in a section "Backwards compatible and does not significantly change benchmark results". See previous release notes for a template.

Also, in future PRs, include a reference to the issue it addresses through a link. For example, by including in the opening comment something like "This is for #704."

@Jaap-Neven Jaap-Neven self-assigned this Dec 4, 2024
@Jaap-Neven
Copy link
Contributor Author

Jaap-Neven commented Dec 4, 2024

Hi @dhblum, Thanks for the feedback!
It seems my preliminary changes for #706 are again included here, this is due to the fact i merged the branch for Issue #582 to my main in my own fork, before i forked this main to the branch of this issue. This is something I will avoid in the future, sorry for this.

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.

2 participants