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

use shorter string for NOSTR value #388

Merged
merged 4 commits into from
Sep 26, 2024

Conversation

braingram
Copy link
Collaborator

@braingram braingram commented Sep 20, 2024

The current NOSTR value used in the maker utils is incompatible with some archive_catalog nvarchar entries. Although this value likely shouldn't ever end up in the archive it prevents adding corresponding minLength keywords to the schemas (to check these nvarchars during schema validation).

This PR set's a new NOSTR value of ?. This was chosen as the smallest nvarchar value is 2 for visit_file_activity.

meta.filename requires a different value none as source_catalog will use this value for some unit tests:
https://github.com/spacetelescope/romancal/blob/96af8bbc6a6dafd48d1beb251b5209d9da875ca1/romancal/source_catalog/source_catalog_step.py#L151
Which then leads to a failure since it attempts to write to "?".

romancal also includes some of these default values in the doctests. spacetelescope/romancal#1419 is needed before this PR can be merged. EDIT: romancal PR is merged, rerunning regtests but will open this for review in case there are requested changes

Regression tests: https://github.com/spacetelescope/RegressionTests/actions/runs/10966346784
show same failures as main: https://github.com/spacetelescope/RegressionTests/actions/runs/10998102193

This PR is a requirement for spacetelescope/rad#448

Tasks

  • update or add relevant tests
  • update relevant docstrings and / or docs/ page
  • Does this PR change any API used downstream? (if not, label with no-changelog-entry-needed)
    • write news fragment(s) in changes/: echo "changed something" > changes/<PR#>.<changetype>.rst (see below for change types)
    • start a romancal regression test with this branch installed ("git+https://github.com/<fork>/roman_datamodels@<branch>")
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

Copy link

codecov bot commented Sep 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.68%. Comparing base (087a60d) to head (7030feb).
Report is 48 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #388      +/-   ##
==========================================
+ Coverage   97.56%   97.68%   +0.12%     
==========================================
  Files          30       37       +7     
  Lines        2788     3372     +584     
==========================================
+ Hits         2720     3294     +574     
- Misses         68       78      +10     

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

Copy link
Collaborator

@PaulHuwe PaulHuwe left a comment

Choose a reason for hiding this comment

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

LGTM

@braingram braingram merged commit 2734f19 into spacetelescope:main Sep 26, 2024
16 checks passed
@braingram braingram deleted the shorter_nostr branch September 26, 2024 20:00
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