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
adding format argument to get_stations_bulk (and what else?) #3173
Conversation
👍 Hmm.. to be honest, just after a brief look at it, I think all parameters that get urlencoded in the GET request should in principle be included here too..!?! So all the latitude/maxradius/... parameters should be there too, I think. Also, while you are at it, if you want you could also handle #3138, i.e. for station request, do not use autodiscovery of file format, but rather use |
Ah, I guess it's OK to base this on master, since it changes the call syntax |
started looking into the ampersand bug... if I understand the solution is to set format='STATIONXML' to the default in the function header and add something like
in read_inventory? which then gives a, i guess more detailed error of
(also adding a seiscomp scXml option as this was always bugging me but perhaps overstepping a smidge) |
@filefolder yes, kind of. I am not sure if adding anything besides the formats StationXML (
|
were we doing a separate pr for this? line 750 clients/fdsn/client.py
I think I find this less informative vs before where it literally points to the problem (py 3.8 anyway). Also maybe relevant that seiscomp will catch and fix these types of errors at inventory import, and with the issue fixed at IRIS, this should now be a rare bug for end users. |
This isn't what would happen in the originally described case. On requesting a "xml" download (format not specified or "xml") this will hit your file: if format is None or format == 'xml':
inventory = read_inventory(data_stream, format='STATIONXML')
I dont care either way, I'd say just do it in one PR |
I think that's it (also put it in get_stations_bulk), but not sure how to test exactly as seiscomp won't allow it and the original example is fixed I think |
I dont think it needs testing, that was just a garbled file and we wanted to have a better error message which we should have now. |
@filefolder like mentioned above, I believe most (if not all) parameters used in the regular request should be available to bulk request in sent via the POST data payload like the The lat/lon circular/box constraints, updatedafter, matchtimeseries need to be accomodated. The time specific constraints don't really make much sense, since start/end time is specified on each line, and just checked IRIS, they refuse to accept requests with any of the time constraints at the top of the list in the POST body as parameters Can you add those too?
|
Also, please add a changelog entry, that checklist in the PR template is not completely useless ;-P |
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.
- add other parameters to bulk POST payload
- changelog
obspy/clients/fdsn/client.py
Outdated
minradius=None, maxradius=None, level=None, | ||
includerestricted=None, includeavailability=None, | ||
updatedafter=None, matchtimeseries=None, filename=None, | ||
format=None, **kwargs) |
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.
please add new kwargs after preexisting kwargs, to prevent breaking existing user code in case of using these kwargs as args (which isn't what you would usually do, but still)
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 will do if you say so.. I did consider that initially but decided it may be better to make a small leap to maintain the same general structure as get_stations
since this jumbled order with format and filename etc in the middle will be permanent after this.
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 agree, it might be slightly ugly, but I think being safe to not break code is more important.
I think we could consider to eventually rectify this by making a leap to introducing , *,
in call syntax, making it def get_stations_bulk(self, bulk, *, minlatitude=None, maxlatitude=None, ...)
to force using these kwargs by their name, which kind of is the only sane way to use them and what I would expect most people do anyway. When/if we do that as an announced potentially breaking change, we would then be free to reorder as we please.
4fde672
to
5a1df81
Compare
Added
format
argument to get_stations_bulk as currently there is no way to save the file as text, only xml (or whatever the server default is). There may be more edits to make here but I don't think so?Fixes #3140