Skip to content

Conversation

@diogomatoschaves
Copy link
Contributor

  • Closes Run verify_schedule at start of fetch command #162

  • Moves _get_schedule and _get_ship_config to utils file.

  • Refactors validation of schedule and ship_config, which become methods of the Schedule and ShipConfig classes respectively.

  • Makes get_instruments_in_schedule a method of the Schedule class

  • Adds more tests for the Schedule and ShipConfig classes.

  • Moves fetch function to its own module

  • Adds schedule verification step at the beginning of the fetch command

  • Tests added

@diogomatoschaves
Copy link
Contributor Author

Hi reviewers, I'm aware that this PR changes more things than the issue it is associated with, but I felt some reorganizing of the code was in order as I was making the changes. If it's too much (in particular the moving of the fetch command function to another module), let me know and I can revert that back.

The commits follow a coherent chronological order, and should be viewed individually to make sense of the changes.

@diogomatoschaves diogomatoschaves changed the title Issue#162 Add schedule verification step to fetch command Mar 6, 2025
@diogomatoschaves diogomatoschaves force-pushed the issue#162 branch 6 times, most recently from 6350175 to 5d1ffdc Compare March 6, 2025 18:44
Copy link
Collaborator

@VeckoTheGecko VeckoTheGecko left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution. Just have some changes, mainly around the fixtures/tests and function signatures/readability

@diogomatoschaves
Copy link
Contributor Author

Hey @VeckoTheGecko I was a bit busy the last 2 weeks, so only now making the requested changes. But here they are! Thanks for the review 👌

Copy link
Collaborator

@VeckoTheGecko VeckoTheGecko left a comment

Choose a reason for hiding this comment

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

All good. Thanks for getting back to this @diogomatoschaves ! Looks good to me now :)

@diogomatoschaves
Copy link
Contributor Author

There was an error in the tests that had slipped through the cracks. Hopefully now all tests should pass

@VeckoTheGecko
Copy link
Collaborator

Thanks!

@VeckoTheGecko VeckoTheGecko merged commit 26c33f6 into Parcels-code:main Mar 28, 2025
10 checks passed
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.

Run verify_schedule at start of fetch command

3 participants