-
-
Notifications
You must be signed in to change notification settings - Fork 408
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
Conversation
… about API key before user has had chance to set it.
e855fc8
to
1244625
Compare
Codecov Report
@@ 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
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
1244625
to
20e7460
Compare
There was a problem hiding this 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.
There was a problem hiding this 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.
astroquery/astrometry_net/core.py
Outdated
Astrometry.net API key not set. You should either set this in the astroquery configuration file using: | ||
|
||
[astrometry_net] | ||
api_key = qwdqwjnoi12ioj |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
Thanks @astrofrog! |
Thanks for wrapping this up @bsipocz! |
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.