Add 'orderby' to TransferClient.task_list #1011
Conversation
This parameter is documented but was missing from the implementation. The exact pattern used here (supporting a list of strings) was used to match other pre-existing TransferClient methods with 'orderby' parameters. It could be expanded (e.g. to `Iterable[str]`) or enhanced in a future change.
The existing tests for `TransferClient.task_list` are old, and use testing tools which predate `globus_sdk._testing`. We can update this to our newer paradigm and move the tests to a dedicated module without much effort. The test payload content has only been changed a small amount, and the tests are preserved with near identical behavior to their earlier incarnation.
This is a simple parametrized test which ensures that `orderby` takes and serializes strings and lists of strings correctly.
sirosen
requested review from
aaschaer,
ada-globus,
derek-globus and
kurtmckee
as code owners
| if isinstance(orderby, str): | ||
| query_params["orderby"] = orderby | ||
| else: | ||
| query_params["orderby"] = ",".join(orderby) |
There was a problem hiding this comment.
I haven't looked up the exact syntax, but the else branch here joins on a comma instead of a space which is specified in the :param doc above
There was a problem hiding this comment.
I think this is right -- but if it's wrong we probably have to fix other method implementations, since I 100% copied this from elsewhere in the module.
I think Transfer accepts multiple ordering criteria comma-separated, like created ASC,terminated DESC. I'll double-check the docs and run a live test or two to see if this is right.
If it is though, it maybe needs slightly more docstring, since I think this is a potential point of confusion with ["created ASC", "terminated DESC"] vs ["created", "ASC"]. 😵💫
There was a problem hiding this comment.
Just live-blogging my process here a little bit, I'm reaching for globus api to check this really easily.
This fails (good!):
$ globus api transfer GET /task_list -Q 'orderby=status,DESC'
and this works:
$ globus api transfer GET /task_list -Q 'orderby=status DESC'
So the space is correct. And then we get to some other things which work, like
$ globus api transfer GET /task_list -Q 'orderby=status DESC,label'
$ globus api transfer GET /task_list -Q 'orderby=status DESC,label ASC'
So I'm pretty sure I have the usage here correct ( 😌 ).
But now I think this accepting a list is potentially confusing, so I think this needs enhanced doc (i.e. at least one complex example).
There was a problem hiding this comment.
I'm with Jake that the docstring can be updated to help clarify this behavior.
There was a problem hiding this comment.
I've added an example usage because I'm not sure what the full scope of the documentation here should be. I don't want to overwhelm a reader, but I want to point them in the right direction.
It's in a fresh commit; let me know if it needs further refinement!
An example is added to the docstring which demonstrates correct use of the orderby parameter for ASC/DESC sorting on a field. This doesn't fully elaborate potential usage, but it guides users towards a likely use-case.
Co-authored-by: Kurt McKee <[email protected]>
I noticed this parameter was missing during some globus-cli work.
Adding it here will allow us to avoid relying on
query_paramsto send it.The existing test data for
task_listwas stored in a pretty old testing format, so I've split this work into three commits rather than one:📚 Documentation preview 📚: https://globus-sdk-python--1011.org.readthedocs.build/en/1011/