-
-
Notifications
You must be signed in to change notification settings - Fork 401
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
Datalink updates #2493
Datalink updates #2493
Conversation
Could you rebase, so we don't have the merges for branch update? |
Codecov Report
@@ Coverage Diff @@
## main #2493 +/- ##
==========================================
+ Coverage 62.91% 62.99% +0.07%
==========================================
Files 133 133
Lines 17313 17291 -22
==========================================
- Hits 10893 10892 -1
+ Misses 6420 6399 -21
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Sure, @bsipocz . As we don't use rebase here, I'm a little unfamiliar with it. Are you looking to squash the |
the rebase would remove those commits rather than squashing them. The trick is to rebase it on the
|
5bd3f6e
to
1e0a52a
Compare
Thanks @bsipocz , I think I figured it out. The history is much cleaner now. |
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.
Actual line reviews are minor nitpicks, however there are 5 related test failures, as well as the xfailing test is still failing, though the mark should have been removed in this PR.
astroquery/alma/core.py
Outdated
self._datalink_url = f"{self._get_dataarchive_url()}{DATALINK_SERVICE_PATH}" | ||
except requests.exceptions.HTTPError as err: | ||
log.debug( | ||
f"ERROR getting the CADC registry: {str(err)}") |
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.
Here and below, too
f"ERROR getting the CADC registry: {str(err)}") | |
f"ERROR getting the ALMA registry: {str(err)}") |
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.
though maybe the whole message needs an update as at the end, if I see it correctly, you don't use the registry.
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.
The entire message was inappropriate. It has been modified.
temp = res.to_table() | ||
if ASTROPY_LT_4_1: | ||
if commons.ASTROPY_LT_4_1: |
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.
👍
I still see the remote test failures, one of them is here as an example:
|
Sorry @bsipocz, disregard. I can reproduce it properly. Fixing now. |
…ore reliable host.
A bit of an embarrassing regression there, but it's fixed now. I've also had to adjust the URL used for the Exception ignored in: <socket.socket fd=14, family=AddressFamily.AF_INET, type=SocketKind.SOCK_STREAM, proto=6, laddr=('172.17.0.2', 42962), raddr=('134.171.46.218', 80)>
Traceback (most recent call last):
File "/usr/local/lib/python3.10/sre_parse.py", line 499, in _parse
subpatternappend = subpattern.append
ResourceWarning: unclosed <socket.socket fd=14, family=AddressFamily.AF_INET, type=SocketKind.SOCK_STREAM, proto=6, laddr=('172.17.0.2', 42962), raddr=('134.171.46.218', 80)> It doesn't hold anything up, but I hope it's not a smell of something deeper. |
astroquery/alma/core.py
Outdated
base_url = self._get_dataarchive_url() | ||
|
||
if base_url.endswith('/'): | ||
self._datalink_url = f'{base_url}{DATALINK_SERVICE_PATH}' | ||
else: | ||
self._datalink_url = f'{base_url}/{DATALINK_SERVICE_PATH}' |
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.
base_url = self._get_dataarchive_url() | |
if base_url.endswith('/'): | |
self._datalink_url = f'{base_url}{DATALINK_SERVICE_PATH}' | |
else: | |
self._datalink_url = f'{base_url}/{DATALINK_SERVICE_PATH}' | |
self._datalink_url = urljoin(self._get_dataarchive_url(), DATALINK_SERVICE_PATH) |
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.
Good suggestion.
perfect, thank you. |
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.
I only have tiny nitpick comments that can be cleaned up next time.
Also, as I see the xfailing test def test_data_info
is still failing, it would be great to fix that up, too.
However, none of the above are critical and as this alma issue has been brought up several times recently, I would rather go ahead and merge this PR now as is.
pytest.fail('Should not get here.') | ||
|
||
|
||
# @patch('pyvo.dal.adhoc.DatalinkService', side_effect=_mocked_datalink_sync) |
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.
Can this be cleaned up?
@@ -127,7 +127,9 @@ You can query by object name or by circular region: | |||
.. doctest-remote-data:: | |||
|
|||
>>> from astroquery.alma import Alma | |||
>>> m83_data = Alma.query_object('M83') | |||
>>> alma = Alma() | |||
>>> alma.archive_url = 'https://almascience.eso.org' # optional to make doctest work |
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.
I wonder whether we should hide this from the documentation e.g. setting it in the config that is being hidden from the rendering, so users are not copy pasting it?
cc @keflavich?
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.
Yeah, this seems unnecessary to include in the documentation since users should default to the default auto-redirect tool?
But otoh, there should be an example of how to change servers because too often one or more are down
from io import StringIO | ||
import os | ||
|
||
import pytest | ||
import requests | ||
import unittest |
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.
I think these unused imports came back accidentally.
Thank you @at88mph! |
Fixes #2489, replacement for #2438