QF refactor and mypy cleanup #635
Conversation
| @@ -24,6 +24,7 @@ def from_api_key(cls, api_key: str, datasets_dir: Optional[Path] = None) -> Meta | |||
| config = DarwinConfig.from_api_key_with_defaults(api_key=api_key) | |||
| client = Client(config) # create a temporary client to get the default team | |||
| token_info = client.get("/users/token_info") | |||
| assert isinstance(token_info, dict) | |||
There was a problem hiding this comment.
overcomes a mypy typing thing
There was a problem hiding this comment.
I think this would've also done it:
token_info = client.get("/users/token_info", dict())but fair play.
| filters = QueryFilter._from_args(*args, **kwargs) | ||
| for item in filters: | ||
| self += item | ||
| return self |
There was a problem hiding this comment.
__add__ function now calls the underlying self.__class__ constructor and passes through all the necessary values, so no longer any need to overwrite where in specific implementations
There was a problem hiding this comment.
Because if there are filters the class instance has become the class in question?
I don't get how this implements our edge cases though, which override where?
There was a problem hiding this comment.
So the problem was that for specific objects, like DatasetQuery, where(...) needs to call the constructor of DatasetQuery rather than cls.init() for mypy typing, all of our implementations basically just looked like
class DatasetQuery(Query[DatasetMeta]):
def where(self, ...) -> Query[T]
filter = QueryFilter(...)
return DatasetQuery(filter)
but using the self.__class__() in the __add__() (or anywhere else we want to implement this), that satisfies mypy's type restraint on the return, while removing a chunk of code we have to write for every query.
| return QueryFilter(name=d["name"], param=str(d["param"]), modifier=modifier) | ||
|
|
||
| @classmethod | ||
| def _from_args(cls, *args: object, **kwargs: str) -> List[QueryFilter]: |
There was a problem hiding this comment.
I think this typing will cause issues in Mypy with calling the function, but using *args: str, **kwargs: int doesn't IIRC.
If I've got this right, the issue with this one, is that invocation will currently work within the function itself, but running something like:
QueryFilter._from_args(c="a", f=2)will lead to a complaint from mypy that c and f aren't objects.
Ignore if this is wrong!
There was a problem hiding this comment.
so object overcomes the mypy typing constraints as everything in python is an object, for mypy it's basically Any but when you actually mean "I accept anything" and not just "I cbf typing this input".
Interestingly, while researching typing for mypy and args/kwargs functions, object and Any are weird, inherited objects of each other. It's a strange, one off relationship that python defines specifically.
In terms of for this though, the intention was that I could provide an array, as well as a dict, but I actually just settled on checking for dict and didn't replace this typing. I could make it stricter and replace with
def _from_args(cls, *args: Dict[str, object], **kwargs: str) -> List[QueryFilter]In your example though the f=2 would cause a typing issue as I've defined only str. There's scope here to allow object instead and then cast to str for the QueryFilter and I'm not fussed, I think explicitly saying str here though does give better type hint about the intention as it's basically signalling that you need to be able to cast it as a str for the filtering object.
There was a problem hiding this comment.
| filters = QueryFilter._from_args(*args, **kwargs) | ||
| for item in filters: | ||
| self += item | ||
| return self |
There was a problem hiding this comment.
Because if there are filters the class instance has become the class in question?
I don't get how this implements our edge cases though, which override where?
| @@ -24,6 +24,7 @@ def from_api_key(cls, api_key: str, datasets_dir: Optional[Path] = None) -> Meta | |||
| config = DarwinConfig.from_api_key_with_defaults(api_key=api_key) | |||
| client = Client(config) # create a temporary client to get the default team | |||
| token_info = client.get("/users/token_info") | |||
| assert isinstance(token_info, dict) | |||
There was a problem hiding this comment.
I think this would've also done it:
token_info = client.get("/users/token_info", dict())but fair play.
Problem
QueryFilter objects are a bit unweildy atm, having to call them with a quite verbose dictionary like syntax
Solution
Added in ability to create filter objects from kwargs while keeping existing functionality, also supports modifier instantiation using 'modifier:value' syntax
Also cleaned up Query code further to handle generalized and inherited
where(*args, **kwargs)that creates these filter objects and doesn't require overwriting in meta query code.Changelog
Streamlined creation and usage of QueryFilters