Extend Gemini to include getting file content and logging out of GOA #2851
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2851 +/- ##
==========================================
+ Coverage 67.49% 67.53% +0.03%
==========================================
Files 233 233
Lines 18420 18444 +24
==========================================
+ Hits 12433 12456 +23
- Misses 5987 5988 +1 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
There are a lot of test failures, those need to be fixed before this can go ahead. Also, will need a changelog entry.
(run pytest -P gemini -R to run the failing remote tests)
Big picture thing, that may not be known outside of a small group is that we're working on moving adding some download utilities to pyvo and plan to use that in astroquery. So it maybe useful for this PR/work, too (but given it's not yet merged, let alone released, I would not wait for it with this PR)
Besides I left a couple of smaller review comments.
| if length == 0: | ||
| log.warn(f'URL {url} has length=0') | ||
|
|
||
| blocksize = astropy.utils.data.conf.download_block_size |
There was a problem hiding this comment.
you need to explicitly import this, importing astropy above is not enough.
| @@ -433,6 +434,97 @@ def get_file(self, filename, *, download_dir='.', timeout=None): | |||
| local_filepath = os.path.join(download_dir, filename) | |||
| self._download_file(url=url, local_filepath=local_filepath, timeout=timeout) | |||
|
|
|||
| def _download_file_content(self, url, timeout=None, auth=None, method="GET", **kwargs): | |||
There was a problem hiding this comment.
Make optional arguments kwarg only. Also, what are the possible keys in kwargs, explicitly list them if possible.
| def _download_file_content(self, url, timeout=None, auth=None, method="GET", **kwargs): | |
| def _download_file_content(self, url, *, timeout=None, auth=None, method="GET", **kwargs): |
There was a problem hiding this comment.
The kwargs could be any accepted keys in the underlying Session.request. I can make a note of them.
| self._authenticated = False | ||
|
|
||
| def get_file_content(self, filename, timeout=None, auth=None, method="GET", **kwargs): | ||
| """Wrapper around `_download_file_content`. |
There was a problem hiding this comment.
Use double backticks for everything that is not sphinx linkable.
| str | ||
| The URL where the file can be downloaded. | ||
| """ | ||
| return f"https://archive.gemini.edu/file/{filename}" |
There was a problem hiding this comment.
This url is hardwired at a lot of places, couldn't ve instead use the conf value, or store it as an instance attribute and use it here and elsewhere, too?
There was a problem hiding this comment.
I agree. I have refactored the gemini module to use conf more and extend the module in a planned future PR.
I did not want to make much changes on this first PR so I could first confirm that the documentation and coding styling matched astroquery.
|
@bsipocz Thanks for the review. I am working on addressing your comments. I have comments below:
|
davner
force-pushed
the
gemini-download-content
branch
from
ba2a654
to
348ef14
Compare
This PR extends the
Geminimodule.get_file_content- Grabs the content of a file, does not save to disk.logout- Deletes the Gemini Observatory Archive cookie.get_file_url- helper method to get the URL for direct file download.I have updated the documentation to include the new functions and included tests.