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

Schedule manager refactor #7352

Closed
wants to merge 82 commits into from
Closed

Schedule manager refactor #7352

wants to merge 82 commits into from

Conversation

Myoldmopar
Copy link
Member

Pull request overview

It's unclear at this point exactly what all this specific PR will include. There will be work to clean up the schedule manager, refactor it, add testing to it, and eventually add multi-year scheduling. This may only be an initial PR to clean up the code before making functional changes that could trigger small diffs.

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.

  • 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
  • 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 to input rules file for interfaces
    • If transition, add transition source
    • If transition, update idfs
  • If new idf included, locally check the err file and other outputs
  • Required documentation updates?
  • ExpandObjects changes?
  • If output changes, including tabular output structure, add to output rules file for interfaces

@Myoldmopar Myoldmopar added the Refactoring Includes code changes that don't change the functionality of the program, just perform refactoring label Jun 20, 2019
@Myoldmopar Myoldmopar added this to the EnergyPlus 9.2.0 milestone Jun 20, 2019
@Myoldmopar Myoldmopar self-assigned this Jun 20, 2019
@nrel-bot-2b
Copy link

@Myoldmopar @lgentile it has been 28 days since this pull request was last updated.

@Myoldmopar Myoldmopar removed the Refactoring Includes code changes that don't change the functionality of the program, just perform refactoring label Jul 26, 2019
@Myoldmopar
Copy link
Member Author

I'm expecting custom_check to fail again, with both a missing IDF in the cmake rules and also the PR is missing labels. But I improved both of those scripts to provide better links and info to the user, after I verify it properly warns with these changes, I'll get rid of those issues and custom_check will pass. Other, more serious test failures are still a little ways away from being resolved.

@Myoldmopar Myoldmopar added Performance Includes code changes that are directed at improving the runtime performance of EnergyPlus Refactoring Includes code changes that don't change the functionality of the program, just perform refactoring labels Jul 27, 2019
@nrel-bot
Copy link

@Myoldmopar @lgentile it has been 28 days since this pull request was last updated.

3 similar comments
@nrel-bot-3
Copy link

@Myoldmopar @lgentile it has been 28 days since this pull request was last updated.

@nrel-bot-2c
Copy link

@Myoldmopar @lgentile it has been 28 days since this pull request was last updated.

@nrel-bot
Copy link

@Myoldmopar @lgentile it has been 28 days since this pull request was last updated.

@nrel-bot-2
Copy link

@Myoldmopar it has been 28 days since this pull request was last updated.

@nrel-bot-2b
Copy link

@Myoldmopar it has been 7 days since this pull request was last updated.

@nrel-bot-2
Copy link

@Myoldmopar it has been 12 days since this pull request was last updated.

@nrel-bot-2b
Copy link

@Myoldmopar it has been 7 days since this pull request was last updated.

4 similar comments
@nrel-bot-2c
Copy link

@Myoldmopar it has been 7 days since this pull request was last updated.

@nrel-bot-2b
Copy link

@Myoldmopar it has been 7 days since this pull request was last updated.

@nrel-bot-2
Copy link

@Myoldmopar it has been 7 days since this pull request was last updated.

@nrel-bot-2
Copy link

@Myoldmopar it has been 7 days since this pull request was last updated.

@nrel-bot-2
Copy link

@Myoldmopar it has been 11 days since this pull request was last updated.

@nrel-bot-2c
Copy link

@Myoldmopar it has been 19 days since this pull request was last updated.

@nrel-bot-2b
Copy link

@Myoldmopar it has been 7 days since this pull request was last updated.

4 similar comments
@nrel-bot-2c
Copy link

@Myoldmopar it has been 7 days since this pull request was last updated.

@nrel-bot-2
Copy link

@Myoldmopar it has been 7 days since this pull request was last updated.

@nrel-bot-2b
Copy link

@Myoldmopar it has been 7 days since this pull request was last updated.

@nrel-bot-2b
Copy link

@Myoldmopar it has been 7 days since this pull request was last updated.

@nrel-bot-2c
Copy link

@Myoldmopar it has been 8 days since this pull request was last updated.

@nrel-bot-2b
Copy link

@Myoldmopar it has been 9 days since this pull request was last updated.

@nrel-bot-2
Copy link

@Myoldmopar it has been 13 days since this pull request was last updated.

@Myoldmopar
Copy link
Member Author

I'm closing this temporarily. I have a new branch that I'll push shortly that addresses the biggest schedule manager fix -- the file reading. Then we can come back to this broader refactor later.

@Myoldmopar Myoldmopar closed this Aug 12, 2020
@Myoldmopar Myoldmopar deleted the ScheduleManagerRefactor branch February 20, 2021 14:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Performance Includes code changes that are directed at improving the runtime performance of EnergyPlus Refactoring Includes code changes that don't change the functionality of the program, just perform refactoring
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants