search

hubmapconsortium / hubmap-api-py-client

Python client for the Cells API
MIT License
1 stars 0 forks source link

Remove unused `values_included` parameter #49

Closed mccalluc closed 3 years ago

mccalluc commented 3 years ago

@SFD5311 : I think this parameter is vestigial... or at least it's not being used now, but maybe it should be?

If it should be referenced in the code, could you come up with a new doctest that fails now, but would be fixed by this change?:

- values_included=[self.query]
+ values_included=values_included

On the other hand: If this PR is good, I think my next step would be to make sort_by a setter method on the object, and then bring back the magic method: If there is no sort, then get_list runs ... if there is a sort, get_details runs.

Thoughts?

SFD5311 commented 3 years ago

I will write up some doc tests that illustrate include_values and sort_by usage. As I understand it, the existing client code passes the original input set as include_values, which I was actually about to file an issue on, because it breaks in cases where the one or more elements of the input set are an expression, i.e. 'Ki67>5000' rather than just an identifier, 'Ki67'. But it also wasn't my intent on the API side to restrict users to only getting values back that were used as filters in the original query.

I'd also be inclined towards allowing the users to get values without sorting, as the latter is a pretty expensive operation, at least as currently implemented. So perhaps include_values and sort_by could both be setter methods, and details could be called if either of them are set?

Follow up: Looks like I'm going to need to do some work on server-side cacheing to make sort_by usable. Maybe this can wait until I get that's done?

mccalluc commented 3 years ago

@sdf -- Sounds good. I'll close this for now, and open a new one that goes in the right direction, but hold off on a larger refactoring. Thanks.