Update fetch and fetch1 syntax#319
Conversation
dimitri-yatsenko
left a comment
dimitri-yatsenko
left a comment
There was a problem hiding this comment.
I approve but I think there are additional opportunities for improvement that we may as well pursue now.
|
|
||
| sql_behavior = update_dict(self.sql_behavior, kwargs) | ||
| ext_behavior = update_dict(self.ext_behavior, kwargs) | ||
| total_behavior = dict(sql_behavior) |
There was a problem hiding this comment.
perhaps we don't need the 'behaviors' dicts anymore since fetch no longer has a state. We could just keep the individual settings in their own variables -- it will be more explicit and more succinct.
There was a problem hiding this comment.
Yes - just note that we are still going to keep backward compatible behavior for a few rounds and hence I'm not drastically changing the internals yet.
datajoint/fetch.py
Outdated
| ret = OrderedDict((name, unpack_(ret[name]) if heading[name].is_blob else ret[name]) | ||
| for name in heading.names) | ||
| else: | ||
| single_output = len(attrs) == 1 |
There was a problem hiding this comment.
single_output assignment can be moved closer to where it's used.
| """ | ||
|
|
||
| def __call__(self, **kwargs): | ||
| def __call__(self, *attrs, **kwargs): |
There was a problem hiding this comment.
Perhaps instead of the anonymous **kwargs we should use named arguments.
There was a problem hiding this comment.
Yeah that would actually be cleaner - probably in next iteration.
| warnings.warn('Use of `squeeze` on `fetch` object is deprecated. Please use `squeeze=True` keyword arguments ' | ||
| 'in the call to `fetch`/`keys` instead', stacklevel=2) | ||
|
|
||
| ret = self.copy() |
There was a problem hiding this comment.
Perhaps we should deprecate FetchBase.copy too. It seems to be needed only for the old stateful Fetch.
There was a problem hiding this comment.
Yes this will be deprecated once statefullness is completely retired.
There was a problem hiding this comment.
I am just saying that it should be marked DEPRECATED like the other deprecated things.
There was a problem hiding this comment.
This usage is only internal hence I didn't put explicit deprecate. I guess I could add that now.
| @property | ||
| def squeeze(self): | ||
| """ | ||
| DEPRECATED |
There was a problem hiding this comment.
Perhaps we could remove FetchBase and Fetch1 and make Fetch do everything. Fetch would then have the argument exactly_one. The fetch1 method could simply call fetch(*args, exactly_one=True, **kwargs). This could obviate the inheritance and the state handling.
There was a problem hiding this comment.
Let's think about it but let's not all of the sudden go about changing everything.
3 participants
Implemented changes discussed in #318, namely:
fetchandfetch1__call__syntax to take in unnamed arguments as attribute namesorder_by,limit,offset,as_dict,squeeze,__getitem__(rel.fetch[]andrel.fetch1[]syntax). Invoking these methods will now throw a deprecation warningwarningsbyloggingas this is not something a library should do!pymysqlare appropriately silencedfetchsyntax while keeping tests for previous syntax until the old syntax is completely removed in a future release.With this major feature addition (i.e. ability to fetch individual attributes with
__call__), the version number has been incremented from0.6.1to0.7.0in accordance to semver.