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

Fixing a query generation for the epn-tap data model. #395

Merged
merged 2 commits into from
Jan 24, 2023

Conversation

msdemlei
Copy link
Contributor

@msdemlei msdemlei commented Jan 9, 2023

This closes bug #393

@codecov
Copy link

codecov bot commented Jan 9, 2023

Codecov Report

❗ No coverage uploaded for pull request base (main@cc97126). Click here to learn what that means.
The diff coverage is n/a.

❗ Current head c7ad518 differs from pull request most recent head 4fc8d11. Consider uploading reports for the commit 4fc8d11 to get more accurate results

@@           Coverage Diff           @@
##             main     #395   +/-   ##
=======================================
  Coverage        ?   79.96%           
=======================================
  Files           ?       52           
  Lines           ?     5969           
  Branches        ?        0           
=======================================
  Hits            ?     4773           
  Misses          ?     1196           
  Partials        ?        0           
Impacted Files Coverage Δ
pyvo/registry/rtcons.py 97.67% <ø> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link

@rsav rsav left a comment

Choose a reason for hiding this comment

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

I have tested this PR and this works for me:
registry.search(registry.Datamodel('epntap'))
now retrieves 244 rows.
Thanks msdemlei !

Comment on lines 649 to 654
# the following test is written this oddly to avoid that extra
# warnings break it.
expectation = set(
['The resource ivo://pyvo/test_regtap.py has multipl'])
assert set(str(w.message)[:50] for w in recwarn
) & expectation == expectation
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this a simpler way to check for the expected warning in the set?

Suggested change
# the following test is written this oddly to avoid that extra
# warnings break it.
expectation = set(
['The resource ivo://pyvo/test_regtap.py has multipl'])
assert set(str(w.message)[:50] for w in recwarn
) & expectation == expectation
assert 'The resource ivo://pyvo/test_regtap.py has multipl' in [str(w.message)[:50] for w in recwarn]

Copy link
Member

Choose a reason for hiding this comment

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

This feels a bit over engineered, any reasons not to use the pytest.warns context in tests?

@msdemlei
Copy link
Contributor Author

msdemlei commented Jan 11, 2023 via email

@andamian andamian self-requested a review January 11, 2023 18:04
Copy link
Contributor

@andamian andamian left a comment

Choose a reason for hiding this comment

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

It looks good except for the style issues.

@bsipocz
Copy link
Member

bsipocz commented Jan 11, 2023

Given this is a user reported bug and also present in the latest release, it should have a changelog entry.

@msdemlei
Copy link
Contributor Author

msdemlei commented Jan 12, 2023 via email

@andamian
Copy link
Contributor

The style check action is still not green in CI. Check https://github.com/astropy/pyvo/actions/runs/3900161110/jobs/6660548272

@msdemlei
Copy link
Contributor Author

msdemlei commented Jan 12, 2023 via email

@bsipocz bsipocz added the bug label Jan 12, 2023
@bsipocz bsipocz added this to the v1.4.1 milestone Jan 12, 2023
@bsipocz
Copy link
Member

bsipocz commented Jan 24, 2023

Given this is all approved, I'll just go ahead and fix the minor issue and changelog conflict and merge it.

@bsipocz bsipocz merged commit bb0a28e into main Jan 24, 2023
@bsipocz
Copy link
Member

bsipocz commented Jan 24, 2023

Thanks @msdemlei for the fix!

@bsipocz bsipocz deleted the fix-epntap-discovery branch January 24, 2023 19:32
@msdemlei
Copy link
Contributor Author

msdemlei commented Jan 25, 2023 via email

bsipocz added a commit that referenced this pull request Mar 7, 2023
Fixing a query generation for the epn-tap data model.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants