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

Astrometry.net: Improvements to API key warnings/errors #2483

Merged
merged 3 commits into from
Aug 17, 2022

Conversation

astrofrog
Copy link
Member

Possible fix for #2482 - only raise an error about missing API key when it is needed, otherwise user doesn't have chance to set it before the warning is emitted.

… about API key before user has had chance to set it.
@codecov
Copy link

codecov bot commented Aug 8, 2022

Codecov Report

Merging #2483 (ed1f080) into main (b1fcfff) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #2483      +/-   ##
==========================================
- Coverage   62.92%   62.91%   -0.02%     
==========================================
  Files         133      133              
  Lines       17302    17308       +6     
==========================================
+ Hits        10888    10889       +1     
- Misses       6414     6419       +5     
Impacted Files Coverage Δ
astroquery/astrometry_net/core.py 47.64% <100.00%> (-2.36%) ⬇️
astroquery/alma/core.py 43.35% <0.00%> (+0.55%) ⬆️

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

@bsipocz bsipocz added this to the v0.4.7 milestone Aug 8, 2022
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.

Thank you, LGTM.

I'll wait until the end of this week to give @mwcraig a chance to review, too, if he wants to.

Copy link
Member

@mwcraig mwcraig 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 fine, though I have one suggestion.

Astrometry.net API key not set. You should either set this in the astroquery configuration file using:

[astrometry_net]
api_key = qwdqwjnoi12ioj
Copy link
Member

Choose a reason for hiding this comment

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

Would it be better to use something that is obviously not a key here? I assume the value here isn't a valid key
but something like XXX or FILL_IN_YOUR_KEY might be better.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok sounds good, I can do that

@bsipocz bsipocz merged commit 68d233a into astropy:main Aug 17, 2022
@bsipocz
Copy link
Member

bsipocz commented Aug 17, 2022

Thanks @astrofrog!

@astrofrog
Copy link
Member Author

Thanks for wrapping this up @bsipocz!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants