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. |
bsipocz
left a comment
There was a problem hiding this comment.
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.
| 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.
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.
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.
The entire message was inappropriate. It has been modified.
|
|
||
| temp = res.to_table() | ||
| if ASTROPY_LT_4_1: | ||
| if commons.ASTROPY_LT_4_1: |
|
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. |
| 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.
| 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) |
perfect, thank you. |
bsipocz
left a comment
There was a problem hiding this comment.
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) |
| >>> 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.
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.
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
|
|
||
| import pytest | ||
| import requests | ||
| import unittest |
There was a problem hiding this comment.
I think these unused imports came back accidentally.
|
Thank you @at88mph! |
Fixes #2489, replacement for #2438