Could you say more here - what does it mean to 'download observations'? Are we downloading FITS images? Raw timestream data? Level X processed files? All of the above?

I wasn't sure if this function was going to be useful or just confusing.

You can achieve the same result using the download_data function with the retrieval_type='OBSERVATION' (which is the default value). Maybe we could delete this function completely.

Answering the question, it retrieves all the products associated with an observation (mostly FITS files).

This might be useful, but it's necessary to explain what "all the products associated with an observation" is likely to mean. Up to you, though.

check how this renders in the docs build; I think you might need different formatting for this to look nice. But I don't remember my .rst well enough to be sure - it might be OK too.

Fixed

Should there be a link to the Herschel mission page here?

Done

Done

it would be helpful to add a few words about what a user can expect to find in an observation tarball, or at least, a link to where one can find that information

Not a problem, I've added a link.

This needs more explanation of how "Observation Products" are different from "data". Is it just the interface that's different, or are the products different?

I've added a few words explaining this.

This section also needs description - what is a postcard? Is it a "cutout" or "quicklook"? What does that mean? Is it usable for science? Again, this could be answered with a link to the right part of the archive, but it would be helpful to have descriptive text here

I've added a brief description.

Can we link to the description of ADQL?

Done

We should have a wrapper function implemented to do this, e.g.:

def query_observations(coordinate, radius, n_obs=10):
     ...

to match the general astroquery user API.

On it 👍

needs the PR number

Done

Please move this module under the esa namespace

Done

please list in the signature all the kwargs you added in the docstring. I also wonder what else could be hidden under **kwargs? If it's not a huge list I think it's better to list them all.

Also, minor preference, I think it's nicer not to list everything in a new line, but I'm ok with if it stays as is.

I've added the parameters in the docstring to the signature. I wouldn't add the rest of them because IMO there are a lot see

I think it's better to define the defaults in the signature, I don't see any practical use here for setting it to None

Makes sense. I've changed it.

These can be in one line, it's easier to read, and having ~100char long lines is totally fine

Done

Why do you need to subclass here?

It's not needed anymore since we're not using the dummyTapHandler anymore.

I don't exactly understand why you need the dummy here and below, these remote test should ideally test using direct connection to the archive, just as a user would do it.

You're right, I think I copy-pasted from the other tests and didn't change it. It's fixed now.

Please use a temp_dir type pytest fixture, as these files are not cleaned up in case there is a test failure. There are examples for this approach is a few other modules, e.g in esasky

I'm using tempfile.TemporaryDirectory() now and I've added a download_dir parameter to all the functions.

users may want to be able to set the download directory (e.g download_dir in esasky), but still let the filenames figured out automatically. Would be nice if this method and a few below would support it.

Added download_dir

These remote tests are very slow (I have let them run for 30mins, 2 failed one success, and cancelled the remaining), I'm not exactly sure about the reasons. If possible please use obs_ids that belongs to smaller files, etc.
While these tests are not running in CI for pull requests, we run them weekly from CI cron, and also run them manually to check for regression, it's really important that they would run in a reasonably quick timescale.

I've changed the observations that were being downloaded to smaller ones and I've also changed the query so it only downloads one product_level. Now the tests take ~10 seconds.

This points outside the scope of this PR, but I this it would make sense API wise it would be better to rename all these query_xyz_tap methods to query_tap. What do you guys, @lvalerom @jespinosaar, think?

I think it makes sense. I would change it in another PR along with the other missions though.

Yes, that should definitely be done in a separate PR, as the other modules need to be renamed using proper deprecations.

nitpick: no linebreaks are needed in these types of strings

I could be wrong but I think these ValueError were not properly formatted.

ValueError("Method '{}' is not invoked. (Invoked method \
                   is '{}'.)").format(method, self._invokedMethod)
# Should be
ValueError("Method '{}' is not invoked. (Invoked method "
                  "is '{}'.)".format(method, self._invokedMethod))
# Notice the parenthesis

I've noticed that this is happening in other missions. I can open a PR to fix this if you would like.

Yes, a PR would be great. Also, these can be now changed to use f-strings, that way there are fewer parens, and a bit more easily legible message.

I've fixed the formatting in this module. I'll open a different PR to address the other modules.

no need for relative import here

Fixed.

nitpick: we only support recent enough pythons, so this can be simplified to super().__init__(). A ton of the modules hasn't been cleaned up, so no need to do it in this PR.