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

Move stnode into a sub-package #213

Merged
merged 14 commits into from
Jun 23, 2023

Conversation

WilliamJamieson
Copy link
Contributor

@WilliamJamieson WilliamJamieson commented Jun 21, 2023

This PR moves stnode into a sub-package and then splits module apart into separate modules each handling one part of the functionality of stnode. In addition to the split, I have adjusted the names of some of the private functions and added further documentation of stnode. Hopefully, the split of responsibilities combined with the additional inline code documentation will make stnode easier to maintain.

In addition, to the split/document, I also simplified a few parts of the code:

  1. I removed the metaclass based registry of node classes, and replaced it with __init_subclass__ registry. The metaclasses are more than we need for this application.
  2. I made all STNode classes dynamically generated (solved scalar/list node corner case).
  3. As part of the dynamic changes, I changed the code to using a mixin based system. That is for nodes that we need to define additional code on, instead of making a direct subclass, instead I changed it to a mixin which is automatically detected and mixed into the class dynamicall.

Checklist

@WilliamJamieson WilliamJamieson requested a review from a team as a code owner June 21, 2023 00:49
@codecov
Copy link

codecov bot commented Jun 21, 2023

Codecov Report

Patch coverage: 91.98% and project coverage change: +0.22 🎉

Comparison is base (b7c92c3) 94.25% compared to head (fabcc6f) 94.48%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #213      +/-   ##
==========================================
+ Coverage   94.25%   94.48%   +0.22%     
==========================================
  Files          18       25       +7     
  Lines        1742     1778      +36     
==========================================
+ Hits         1642     1680      +38     
+ Misses        100       98       -2     
Impacted Files Coverage Δ
src/roman_datamodels/stnode/_tagged.py 87.30% <87.30%> (ø)
src/roman_datamodels/stnode/_node.py 87.97% <87.97%> (ø)
src/roman_datamodels/stnode/_factories.py 96.07% <96.07%> (ø)
src/roman_datamodels/stnode/__init__.py 100.00% <100.00%> (ø)
src/roman_datamodels/stnode/_converters.py 100.00% <100.00%> (ø)
src/roman_datamodels/stnode/_mixins.py 100.00% <100.00%> (ø)
src/roman_datamodels/stnode/_registry.py 100.00% <100.00%> (ø)
src/roman_datamodels/stnode/_stnode.py 100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Collaborator

@perrygreenfield perrygreenfield left a comment

Choose a reason for hiding this comment

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

Other than a minor docstring error, it looks good. I had to refresh my brain by looking at the original code (even though I wrote much of it, but not the dynamic aspects) and that took some time.

----------
ctx :
An ASDF file context.
tag_uri : str
Copy link
Collaborator

Choose a reason for hiding this comment

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

This doesn't match the function signature

Copy link
Collaborator

@perrygreenfield perrygreenfield left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@perrygreenfield perrygreenfield left a comment

Choose a reason for hiding this comment

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

LGTM

@WilliamJamieson WilliamJamieson merged commit 5c33610 into spacetelescope:main Jun 23, 2023
@WilliamJamieson WilliamJamieson deleted the refactor/stnode branch June 23, 2023 17:57
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.

2 participants