WIP: Model properties #96
Conversation
googlebot
added
the
cla: yes
src/google/cloud/ndb/model.py
Outdated
| """Dynamically get a subproperty.""" | ||
| # Optimistically try to use the dict key. | ||
| prop = self._modelclass._properties.get(attrname) | ||
| # We're done if we have a hit and _code_name matches. |
There was a problem hiding this comment.
Comment seems unrelated to code. Don't see _code_name anymore. Only done-ness seems to be if we didn't get a hit, then it raises an exception.
src/google/cloud/ndb/model.py
Outdated
| return prop_copy | ||
|
|
||
| def _comparison(self, op, value): | ||
| if op != '=': |
src/google/cloud/ndb/model.py
Outdated
| raise exceptions.BadFilterError( | ||
| 'Cannot query for non-empty repeated property %s' % prop._name) | ||
| continue | ||
| assert isinstance(vals, list) and len(vals) == 1, repr(vals) |
There was a problem hiding this comment.
Is it possible for user or programmer error to cause this to be False?
src/google/cloud/ndb/model.py
Outdated
| 'Expected list, tuple or set, got %r' % (value,)) | ||
| from .query import DisjunctionNode, FalseNode | ||
| # Expand to a series of == filters. | ||
| filters = [self._comparison('=', val) for val in value] |
src/google/cloud/ndb/model.py
Outdated
| (self._modelclass.__name__, value.__class__)) | ||
|
|
||
| def _has_value(self, entity, rest=None): | ||
| # rest: optional list of attribute names to check in addition. |
There was a problem hiding this comment.
Turn comment into docstring?
src/google/cloud/ndb/model.py
Outdated
| super(GenericProperty, self).__init__(name=name, **kwargs) | ||
| self._compressed = compressed | ||
| if compressed and self._indexed: | ||
| # TODO: Allow this, but only allow == and IN comparisons? |
There was a problem hiding this comment.
We've been getting rid of ancient TODO comments. If it hasn't been done by now...
If something feels particularly pertinent, you can always make an issue in github, which will be a better way to track/prioritize.
|
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the ℹ️ Googlers: Go here for more info. |
googlebot
added
cla: no
|
@cguardia Can you try the following on your local checkout? I'm assuming you have two remotes, $ git checkout master && git fetch --all --prune && git merge upstream master
$ git checkout model_properties
$ git rebase -i master # edit out the commits before 6e75bba
$ git push -f origin model_properties |
|
@tseaver seems that it worked, thanks. |
cguardia
added
cla: yes
|
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
googlebot
removed
the
cla: yes
|
|
cguardia
dismissed
chrisrossi’s stale review
Changes have been applied and reviewer is on vacation.
src/google/cloud/ndb/model.py
Outdated
| self._modelclass = modelclass | ||
|
|
||
| def _get_value(self, entity): | ||
| """Override _get_value() to *not* raise UnprojectedPropertyError.""" |
There was a problem hiding this comment.
Could you expand this docstring to say something about why this intentionally raises unprojectedpropertyerror and under what circumstances you'd want to override that behavior?
googlebot
added
the
cla: yes
cguardia
added
the
kokoro:force-run
yoshi-kokoro
removed
the
kokoro:force-run
This is almost done, but needs a final push for test coverage edge cases. I decided to remove work on LocalStructuredProperty, so that this can be merged first, and after that I can handle PR #93 without conflicts.
For now, I commented out the part where the RepeatedStructuredPropertyFilterPredicate is used.
Note that the StructuredProperty had quite complex serialization/deserialization methods, which we do not support now, so not sure what is needed to be done there.