Skip to content

Fix optional deps #292

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.

Sign up for GitHub

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

Jump to bottom
Merged
ocefpaf merged 13 commits into from
Dec 8, 2022
Merged

Fix optional deps #292

ocefpaf merged 13 commits into from
Dec 8, 2022

Conversation

@ocefpaf
Copy link
Member

@ocefpaf ocefpaf commented Dec 7, 2022 •
edited
Loading

Closes #291 and incorporates #276.

@ocefpaf ocefpaf mentioned this pull request Dec 7, 2022
Closed
5 tasks
@ocefpaf ocefpaf marked this pull request as ready for review December 8, 2022 00:05
@ocefpaf ocefpaf requested a review from abkfenris December 8, 2022 00:05
abkfenris
abkfenris approved these changes Dec 8, 2022
View reviewed changes
Copy link
Contributor

@abkfenris abkfenris left a comment

Choose a reason for hiding this comment

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

I think it looks ok, but it may be easier to tell when we do have failures if the steps in the core test are broken apart.

It may also be worth combining the core tests in as a new job in the current tests.yml, as it can be a bit quicker to navigate between different jobs in the same workflow than between jobs in different workflows.

@ocefpaf @abkfenris
split install
3c348fa 
Co-authored-by: Alex Kerney <abk@mac.com>
@ocefpaf
Copy link
Member Author

ocefpaf commented Dec 8, 2022

It may also be worth combining the core tests in as a new job in the current tests.yml, as it can be a bit quicker to navigate between different jobs in the same workflow than between jobs in different workflows.

I don't have a strong opinion on this but I'm slightly more inclined to multiple files than a huge one. Sure, it won't be a huge one right away but they can grow in size. In this cases the context is not only a small subset without the optional deps but also a non-conda setup.

@ocefpaf
Copy link
Member Author

ocefpaf commented Dec 8, 2022

I'll work on the docs in another PR. Let's get this one in.

@ocefpaf ocefpaf merged commit f11ad16 into ioos:main Dec 8, 2022
@ocefpaf ocefpaf deleted the fix_optional_deps branch December 8, 2022 14:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@abkfenris abkfenris abkfenris approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Refactor tests and mandatory vs optional dependencies

2 participants

@ocefpaf @abkfenris