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

DM-44878: Fill in the TAP_SCHEMA size attribute #83

Merged
merged 3 commits into from
Jul 23, 2024

Conversation

JeremyMcCormick
Copy link
Collaborator

@JeremyMcCormick JeremyMcCormick commented Jul 5, 2024

  • Set the size attribute based on the value of votable_arraysize from the column
    • If arraysize is an int in the YAML data or a string that is parseable as an int, set size=arraysize
    • If arraysize has the form "N*" where N is an int, set size=N
    • For all other cases, size will default to None which will result in a null being inserted. This includes the case where arraysize is * or a blank string.
  • Change the votable_arraysize field on the Pydantic model to accept a string or an integer
    • The field was too restrictive based on the valid values from the IVOA VOTable standard.
  • Fix bad type hint in test_tap

@JeremyMcCormick JeremyMcCormick requested a review from gpdf July 5, 2024 22:13
@JeremyMcCormick JeremyMcCormick marked this pull request as draft July 5, 2024 22:13
@JeremyMcCormick JeremyMcCormick force-pushed the tickets/DM-44878 branch 2 times, most recently from 0568f0d to cf20718 Compare July 5, 2024 22:17
@JeremyMcCormick JeremyMcCormick changed the title DM-44878: Fill in the size attribute and make a few additional improvements and corrections to the TAP module and its tests DM-44878: Fill in the size attribute and make a few additional minor improvements and corrections to the TAP module and its tests Jul 5, 2024
Copy link

codecov bot commented Jul 5, 2024

Codecov Report

Attention: Patch coverage is 33.33333% with 14 lines in your changes missing coverage. Please review.

Project coverage is 90.71%. Comparing base (cc76deb) to head (e01a0a2).

Files Patch % Lines
python/felis/tap.py 22.22% 13 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #83      +/-   ##
==========================================
- Coverage   91.47%   90.71%   -0.77%     
==========================================
  Files          14       14              
  Lines        1643     1659      +16     
  Branches      347      352       +5     
==========================================
+ Hits         1503     1505       +2     
- Misses         84       97      +13     
- Partials       56       57       +1     

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

@JeremyMcCormick JeremyMcCormick force-pushed the tickets/DM-44878 branch 3 times, most recently from 2302147 to 2ba31af Compare July 10, 2024 17:17
@JeremyMcCormick JeremyMcCormick marked this pull request as ready for review July 10, 2024 17:17
@JeremyMcCormick JeremyMcCormick changed the title DM-44878: Fill in the size attribute and make a few additional minor improvements and corrections to the TAP module and its tests DM-44878: Fill in the size attribute and make a few related minor improvements and corrections Jul 10, 2024
@JeremyMcCormick JeremyMcCormick changed the title DM-44878: Fill in the size attribute and make a few related minor improvements and corrections DM-44878: Fill in the TAP_SCHEMA size attribute and make a few related minor improvements and corrections Jul 10, 2024
Copy link

@gpdf gpdf left a comment

Choose a reason for hiding this comment

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

One requested change before merging, see regular expression rewording

python/felis/tap.py Outdated Show resolved Hide resolved
@JeremyMcCormick JeremyMcCormick changed the title DM-44878: Fill in the TAP_SCHEMA size attribute and make a few related minor improvements and corrections DM-44878: Fill in the TAP_SCHEMA size attribute Jul 12, 2024
Copy link

@gpdf gpdf left a comment

Choose a reason for hiding this comment

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

Looks good. Future work on testing is indicated, though.

@JeremyMcCormick JeremyMcCormick marked this pull request as draft July 22, 2024 16:33
@JeremyMcCormick
Copy link
Collaborator Author

This needs to be fixed up due to changes from #89.

@JeremyMcCormick JeremyMcCormick marked this pull request as ready for review July 22, 2024 21:15
@JeremyMcCormick
Copy link
Collaborator Author

This should be fixed now.

The type hint should be `Schema` and not a `MutableMapping`.
The existing type hints were too restrictive based on the valid values
for arraysize from the IVOA VOTable standard.
Use the value for arraysize to set the size attribute in the TAP data.
@JeremyMcCormick JeremyMcCormick merged commit 9349df8 into main Jul 23, 2024
13 of 15 checks passed
@JeremyMcCormick JeremyMcCormick deleted the tickets/DM-44878 branch July 23, 2024 15:45
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