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

Correct some IDD/IDF issues #6238

Merged
merged 4 commits into from
Aug 14, 2017
Merged

Correct some IDD/IDF issues #6238

merged 4 commits into from
Aug 14, 2017

Conversation

mitchute
Copy link
Collaborator

@mitchute mitchute commented Aug 12, 2017

See issue #5977

Please change this line to a description of the pull request, with useful supporting information including whether it is a new feature, or fixes a defect, a cross reference to any defects addressed in this PR, the type of changes to be made, and whether diffs are expected/justified based on this change.

Work Checklist

Add to this list or remove from it as applicable. This is a simple templated set of guidelines.

  • Title of PR should be user-synopsis style (clearly understandable in a standalone changelog context)
  • At least one of the following appropriate labels must be added to this PR to be consumed into the changelog:
    • Defect: This pull request repairs a github defect issue. The github issue should be referenced in the PR description
    • Refactoring: This pull request includes code changes that don't change the functionality of the program, just perform refactoring
    • NewFeature: This pull request includes code to add a new feature to EnergyPlus
    • Performance: This pull request includes code changes that are directed at improving the runtime performance of EnergyPlus
    • DoNoPublish: This pull request includes changes that shouldn't be included in the changelog

Review Checklist

This will not be exhaustively relevant to every PR.

  • Code style (parentheses padding, variable names)
  • Functional code review (it has to work!)
  • If defect, results of running current develop vs this branch should exhibit the fix
  • CI status: all green or justified
  • Performance: CI Linux results include performance check -- verify this
  • Unit Test(s)
  • C++ checks:
    • Argument types
    • If any virtual classes, ensure virtual destructor included, other things
  • IDD changes:
    • Verify naming conventions and styles, memos and notes and defaults
    • Open windows IDF Editor with modified IDD to check for errors
    • If transition, add rules to spreadsheet
    • If transition, add transition source
    • If transition, update idfs
  • If new idf included, locally check the err file and other outputs
  • Documentation changes in place
  • Changed docs build successfully
  • ExpandObjects changes?
  • If output changes, including tabular output structure, add to output rules file for interfaces

@mitchute mitchute self-assigned this Aug 12, 2017
@Myoldmopar
Copy link
Member

After you can verify that those files run transition properly, this is good to go in.

Copy link
Member

@Myoldmopar Myoldmopar left a comment

Choose a reason for hiding this comment

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

👍

@mjwitte mjwitte added Defect Includes code to repair a defect in EnergyPlus IDDChange Code changes impact the IDD file (cannot be merged after IO freeze) labels Aug 14, 2017
@mjwitte mjwitte modified the milestone: EnergyPlus 8.8.0 Aug 14, 2017
@mitchute
Copy link
Collaborator Author

@Myoldmopar this is ready for final review and merge.

@Myoldmopar Myoldmopar merged commit 56d4e65 into develop Aug 14, 2017
@Myoldmopar Myoldmopar deleted the IDD-IDF-Issues branch August 14, 2017 19:28
@Myoldmopar Myoldmopar mentioned this pull request Aug 14, 2017
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Defect Includes code to repair a defect in EnergyPlus IDDChange Code changes impact the IDD file (cannot be merged after IO freeze)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants