Fix the FlowsClient.get_run() include_flow_description parameter
#778
Conversation
kurtmckee
requested review from
ada-globus,
derek-globus,
sirosen and
aaschaer
as code owners
There was a problem hiding this comment.
The change looks good for what it is, but is fixing this client-side the correct approach?
I would prefer that we allow for the "normal python encoding" (requests) of bools for any bool parameters in our APIs. I know that's conforming our services to a particular client, but a pretty important one for our usages.
If we fix the API, will we then revert this change in order to simplify the client?
| consolidated_query_params = query_params or {} | ||
| if include_flow_description is not None: | ||
| value = str(include_flow_description).lower() # "true" or "false" | ||
| consolidated_query_params["include_flow_description"] = value | ||
|
|
||
| return self.get( | ||
| f"/runs/{run_id}", | ||
| query_params={ | ||
| "include_flow_description": include_flow_description, | ||
| **additional_query_params, | ||
| }, | ||
| query_params=consolidated_query_params, | ||
| ) |
There was a problem hiding this comment.
Is there a reason not to simply call it query_params?
i.e.
if query_params is None:
query_params = {}(or inline that if line count is the enemy 😉 )
There was a problem hiding this comment.
is fixing this client-side the correct approach
Yes, because requests does not produce the documented values, and responses won't match unless the capitalization is correct.
Fixed
FlowsClient.get_run()include_flow_descriptionparameterso it is sent as a lowercase string (
trueorfalse).📚 Documentation preview 📚: https://globus-sdk-python--778.org.readthedocs.build/en/778/