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

[SIMBAD] Add emtpy response warning #3068

Merged
merged 2 commits into from
Jul 16, 2024

Conversation

ManonMarchand
Copy link
Member

Hi astroquery devs,

This PR adds a NoResultWarning when a SIMBAD query returns an empty table. This should fix #3060 .

@ManonMarchand ManonMarchand changed the title Add emtpy response warning [SIMBAD] Add emtpy response warning Jul 16, 2024
@bsipocz bsipocz added this to the v0.4.8 milestone Jul 16, 2024
Copy link
Member

@bsipocz bsipocz left a comment

Choose a reason for hiding this comment

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

This looks good thank you.

I see the doctest failing locally, but it's some weird issue that I don't yet tracked down as it not always showing up in CI. So I'll just leave it as is now.

Thanks!

@bsipocz bsipocz merged commit 53e636f into astropy:main Jul 16, 2024
7 of 9 checks passed
@ManonMarchand
Copy link
Member Author

Oh? Could you show your local failure? I don't have any.

@bsipocz
Copy link
Member

bsipocz commented Jul 17, 2024

It's just messing around ELLIPSIS and whitespace, it really is a local issue most of the time.

But for this one, I see it in CI, too: https://github.com/astropy/astroquery/actions/runs/9814474694/job/27525866043#step:5:13186

(just restarted this run after merging a bunch of PRs this morning, so any failures your see there are on main)

@ManonMarchand
Copy link
Member Author

Maybe I have something different in my config, because I see the failure the other way, and I patched the width of the output in 9f65c6f
I'll have a look on my side too

@ManonMarchand ManonMarchand deleted the add-emtpy-response-warning branch July 17, 2024 07:01
@bsipocz
Copy link
Member

bsipocz commented Jul 17, 2024

I would say don't worry about it for now (or we should just patch things up for the weekly CI to have fewer issues, but then again it doesn't really matter. I'll open an issue upstream for doctestplus as ideally, we should handle it within it.

(Also, maybe you have an older pytest-doctestplus version? If yes, that may help to narrow down when and why this became a problem).

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.

Handling No astronomical object found response
2 participants