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

Remove stream config #100

Merged
merged 3 commits into from
Mar 16, 2024
Merged

Remove stream config #100

merged 3 commits into from
Mar 16, 2024

Conversation

ocefpaf
Copy link
Member

@ocefpaf ocefpaf commented Mar 2, 2023

Start to address #99

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@ocefpaf ocefpaf marked this pull request as ready for review March 14, 2024 19:14
@ocefpaf
Copy link
Member Author

ocefpaf commented Mar 14, 2024

@kthyng sorry to ping you here but who would be the best person to review this one?

@kthyng
Copy link

kthyng commented Mar 14, 2024

@ocefpaf No problem! We're discussing internally about getting some people involved.

@iwensu0313
Copy link
Contributor

iwensu0313 commented Mar 15, 2024

Hi @ocefpaf - thanks for taking a stab here. @melaniefeen and I took a look at it together. These changes to documentation look good.

We did notice that StreamConfig is still referenced in the following places:

But StreamConfig objects don't seem to exist anymore in the most recent version of the ioos_qc package. Digging around to figure out what the replacement is. Perhaps just Config? Any ideas?

}
c = StreamConfig(config)


ContextConfig
~~~~~~~~~~~~~
A ``ContextConfig`` object defines multiple ``StreamConfig`` objects as well as optional `region` and `window` objects.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should StreamConfig be replaced w/ something else here? What's the new object called?

Copy link
Member Author

Choose a reason for hiding this comment

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

ContextConfig can take any ConfigType if I understood the code correctly. I changed to that instead of StreamConfig.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yea I see that now, ConfigType object is what we're passing when initializing ContextConfig class.

@ocefpaf
Copy link
Member Author

ocefpaf commented Mar 15, 2024

But StreamConfig objects don't seem to exist anymore in the most recent version of the ioos_qc package. Digging around to figure out what the replacement is. Perhaps just Config? Any ideas?

Quoting Kyle from #99 (comment):

The docs need to be updated, the only interface you need to load any config object is the Config class: from ioos_qc.config import Config

My guess is that this is already documented, right? So it is a matter of just removing StreamConfig. I'll remove it from the places you pointed out.

@iwensu0313
Copy link
Contributor

Looks good to me! @ocefpaf

@ocefpaf ocefpaf merged commit c8d7caa into ioos:main Mar 16, 2024
9 checks passed
@ocefpaf ocefpaf deleted the remove_StreamConfig branch March 16, 2024 12:01
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.

3 participants