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

Refactor directive IDs in the scheduler #1509

Merged
merged 10 commits into from
Aug 6, 2024

Conversation

JoelCourtney
Copy link
Contributor

@JoelCourtney JoelCourtney commented Jul 18, 2024

Description

This contains a couple behavioral changes:

  • All activity directive id mapping or transforming has been removed, except for the upload step where directive ids might not match what they ultimately become in the database.
  • Generated activities are no longer assigned fake directive IDs. They are not directives, so this was unsound.

And several renames / trivial refactors:

  • Renamed SimulatedActivity[Id] to ActivityInstance[Id] to better contrast with ActivityDirective.
  • Deleted SchedulingActivityDirectiveId and replaced all usages with ActivityDirectiveId.
  • Renamed SchedulingActivityDirective to SchedulingActivity because it also holds instance information.
  • Renamed MerlinService and its implementors to MerlinDatabaseService because merlin is not the database.

Verification

All tests still pass. Please suggest any new tests you think would be helpful.

Documentation

I don't think we had any internal documentation of the ID maps, which is part of what made them so annoying. So none needs to be updated!

Future work

Copy link
Collaborator

@mattdailis mattdailis left a comment

Choose a reason for hiding this comment

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

A lot of very sensible changes here. Looks good to me based on visual inspection - if you can get the tests pass I'd say ship it

@mattdailis mattdailis merged commit a505cf9 into develop Aug 6, 2024
6 checks passed
@mattdailis mattdailis deleted the refactor/scheduling-ids-joel branch August 6, 2024 00:27
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.

Refactor directive ids in scheduler
2 participants