Skip to content

Conversation

@WilliamJamieson
Copy link
Collaborator

This PR fixes a couple of things that I noticed while doing the mypy PR #566. These didn't fit with that theme nor do they fit anywhere else.

Tasks

  • Update or add relevant roman_datamodels tests.
  • Update relevant docstrings and / or docs/ page.
  • Does this PR change any API used downstream? (If not, label with no-changelog-entry-needed.)
News fragment change types:
  • changes/<PR#>.feature.rst: new feature
  • changes/<PR#>.bugfix.rst: fixes an issue
  • changes/<PR#>.doc.rst: documentation change
  • changes/<PR#>.removal.rst: deprecation or removal of public API
  • changes/<PR#>.misc.rst: infrastructure or miscellaneous change

@WilliamJamieson WilliamJamieson requested a review from a team as a code owner September 19, 2025 18:52
@codecov
Copy link

codecov bot commented Sep 19, 2025

Codecov Report

❌ Patch coverage is 90.65421% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 95.53%. Comparing base (087a60d) to head (46ad2c6).
⚠️ Report is 197 commits behind head on main.

Files with missing lines Patch % Lines
src/roman_datamodels/stnode/_tagged.py 88.57% 4 Missing ⚠️
src/roman_datamodels/datamodels/_core.py 77.77% 2 Missing ⚠️
src/roman_datamodels/stnode/_mixins.py 86.66% 2 Missing ⚠️
src/roman_datamodels/datamodels/_utils.py 75.00% 1 Missing ⚠️
src/roman_datamodels/stnode/_node.py 90.90% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #568      +/-   ##
==========================================
- Coverage   97.56%   95.53%   -2.03%     
==========================================
  Files          30       41      +11     
  Lines        2788     4279    +1491     
==========================================
+ Hits         2720     4088    +1368     
- Misses         68      191     +123     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Collaborator

@braingram braingram left a comment

Choose a reason for hiding this comment

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

Overall LGTM but docs are failing would you take a look at what's causing the failure? Also would you run the regtests with this branch when it's updated to fix the docs?

@braingram
Copy link
Collaborator

Thanks. The comment in e31f20c looks great. Would you run the regtests with this PR?

@WilliamJamieson
Copy link
Collaborator Author

@WilliamJamieson WilliamJamieson force-pushed the clean_up_mypy branch 2 times, most recently from 7f49d7c to c405188 Compare September 22, 2025 18:42
@WilliamJamieson WilliamJamieson force-pushed the clean_up_mypy branch 2 times, most recently from 4f58ba7 to c54ba10 Compare September 24, 2025 17:09
@WilliamJamieson
Copy link
Collaborator Author

@braingram commit 1a47c73 of this PR is simply what I linked to in #569 (comment). I am happy for you to us this for #569, or some other similar change or we can simply include it as part of this PR. It just made sense to rebase these changes on those as otherwise the merge conflicts will get annoying to deal with.

@braingram
Copy link
Collaborator

@braingram commit 1a47c73 of this PR is simply what I linked to in #569 (comment). I am happy for you to us this for #569, or some other similar change or we can simply include it as part of this PR. It just made sense to rebase these changes on those as otherwise the merge conflicts will get annoying to deal with.

Thanks. I haven't had time to address the requested changes in #569. I have no objection if you updated this PR with a changelog describing the changes in 1a47c73 and I'll close #569.

@WilliamJamieson
Copy link
Collaborator Author

@braingram commit 1a47c73 of this PR is simply what I linked to in #569 (comment). I am happy for you to us this for #569, or some other similar change or we can simply include it as part of this PR. It just made sense to rebase these changes on those as otherwise the merge conflicts will get annoying to deal with.

Thanks. I haven't had time to address the requested changes in #569. I have no objection if you updated this PR with a changelog describing the changes in 1a47c73 and I'll close #569.

Done.

@WilliamJamieson WilliamJamieson force-pushed the clean_up_mypy branch 5 times, most recently from ae34c5c to f18e152 Compare October 1, 2025 18:36
Copy link
Collaborator

@braingram braingram left a comment

Choose a reason for hiding this comment

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

Just one question about the empty __init__.

@WilliamJamieson WilliamJamieson force-pushed the clean_up_mypy branch 2 times, most recently from 02b6414 to 751e567 Compare October 2, 2025 19:53
Copy link
Collaborator

@braingram braingram left a comment

Choose a reason for hiding this comment

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

LGTM. I'll retrigger the regtests than merge once those pass.

@braingram braingram merged commit 11c1bfe into spacetelescope:main Oct 10, 2025
19 of 21 checks passed
@WilliamJamieson WilliamJamieson deleted the clean_up_mypy branch October 13, 2025 15:26
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