Skip to content

Conversation

@dmendelowitz
Copy link
Contributor

Summary

checkLogFile and getEffectiveFromDate have been moved from app.js to RunInstanceLogger.js

New behavior

The client should behave the same as before

Code changes

  • checkLogFile() and getEffectiveFromDate() have been removed from app.js
  • the functionality of checkLogFile has been moved to the constructor of RunInstanceLogger
  • getEffectiveFromDate() moved to RunInstanceLogger
  • app.js changed to use RunInstanceLogger for these functions
  • Tests moved from app.test.js to the new runInstanceLogger.test.js and modified to work with the changes

Testing guidance

  • Ensure that the log file functionality still works as before
  • Check that the changes to the tests make sense and still pass

@dmendelowitz dmendelowitz force-pushed the consolidate-runinstancelogger branch from 3ff9838 to 28bd236 Compare June 17, 2021 19:08
@Dtphelan1 Dtphelan1 self-assigned this Jun 18, 2021
Copy link
Contributor

@Dtphelan1 Dtphelan1 left a comment

Choose a reason for hiding this comment

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

This looks good to me! The one thing this makes me think, however, is that we might want to create super small PR's on other repos to remove their duplicate code and get them to use the RunInstanceLogger for this as well. Let me know what you think about that, or if that's a bit outside the scope of what you were hoping to do for this.

Copy link
Contributor

@julianxcarter julianxcarter left a comment

Choose a reason for hiding this comment

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

This looks good! I also agree that some companion PRs for the dependent repos would be a good next step if that's not out of scope for this task.

@dmendelowitz
Copy link
Contributor Author

I had planned to do the companion PRs for the other repos, just wanted to get this merged in first to make things easier. Now that this is approved, I'll get started on those

@dmendelowitz dmendelowitz merged commit 641b6bb into develop Jun 18, 2021
@dmendelowitz dmendelowitz deleted the consolidate-runinstancelogger branch June 18, 2021 16:21
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.

4 participants