Repository navigation
Refactor: move verification methods up to Expedition class - #410
Conversation
…ing whether schedule or instr config; use more helper functions
j-atkins
changed the title
Expedition classExpedition class
j-atkins
changed the title
Expedition classExpedition class
erikvansebille
left a comment
erikvansebille
left a comment
There was a problem hiding this comment.
Looks good - a few comments below
| def verify_schedule( | ||
| self, | ||
| ignore_land_test: bool = False, | ||
| *, |
There was a problem hiding this comment.
What is this star needed for?
There was a problem hiding this comment.
This is so that every parameter after it is is keyword-only, intended to make the calls more readable. Given we don't do this consistently across the codebase though I'll remove it (done in 200fe42) to avoid confusion/keep it separate from this PR.
|
|
||
| `ship_speed` overrides the configured speed, for checking a prospective speed that has not been saved yet (e.g. in `plan` app). | ||
| """ | ||
| print("\nVerifying route... ") |
There was a problem hiding this comment.
Should this then be "Verifying expedition"? Or would it still make more sense to refer to the "route" here, from a users perspective?
There was a problem hiding this comment.
Yes I suppose "expedition" is better terminology, will change!
| """ | ||
| Verify the feasibility and correctness of the expedition's schedule. | ||
|
|
||
| `ship_speed` overrides the configured speed, for checking a prospective speed that has not been saved yet (e.g. in `plan` app). |
There was a problem hiding this comment.
Make clear that the provided ship_speed should (also) be in knots?
| def verify(self, instruments_in_expedition: Collection[InstrumentType]) -> None: | ||
| """ | ||
| Verify instrument configurations against the schedule. | ||
| Verify instrument configurations against the instruments actually being deployed. |
There was a problem hiding this comment.
Why the extra space before "instrument" here?
There was a problem hiding this comment.
Just a typo from making edits, now fixed.
Note
This PR is part of a stack of three related PRs (#410, #411 , #412 ). Each one builds on the one before it. I will request reviews for each in turn.
This PR refactors the schedule and instrument verification methods, moving them up to the
Expeditionclass level, which owns all three of the schedule, ship and instruments config classes, which are all used in the schedule verification.Therefore,
schedule.verify(ship_speed, instruments_config, ...)becomesexpedition.verify_schedule(...)andinstruments_config.verify(...)becomesexpedition.verify_instruments_config().Also splits the long verification methods into different helpers.
This step is taken to make subsequent PRs in the stack easier to implement (i.e. #411 and #412).