-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Spanner: Partial range test and bug fix #4515
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Spanner: Partial range test and bug fix #4515
Conversation
508d4ae to
c3343dd
Compare
c3343dd to
6c01759
Compare
|
@chemelnucfin I'm not in favor of |
|
@tseaver Also, if you don't supply a begin key or an end key, the constructor specifies one for you. So if you don't spply either, it should specify both? |
3c7a227 to
cfe9062
Compare
|
@tseaver PTAL, thanks. |
cfe9062 to
8f1ebe9
Compare
|
@chemelnucfin I'm still opposed to dropping the ValueError in the constructor: application code is responsible for constructing the |
|
@tseaver My main concern is that there is a difference in behavior if you don't specify either a start or an end key, it gets filled out automatically, and if you don't specify both start and end key it raises ValueError. I think either both cases should raise ValueError, or both should get them filled out automatically. |
|
Specifying a range with only one of @lukesneeringer, @dhermes Can one of you chime in? |
|
ISTM @tseaver is right on here. |
|
I still somewhat disagree, since a However, I can understand your point that you shouldn't construct a plain Anyway, my suggestion would be, either to document this behavior, or convert an empty |
|
BTW, I did change the empty constructor back to the way it was. |
bba3420 to
1b76a11
Compare
| kwargs['start_open'] = _make_list_value_pb(self.start_open) | ||
|
|
||
| if self.start_closed: | ||
| elif self.start_closed is not None: |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
| elif self.start_closed is not None: | ||
| kwargs['start_closed'] = _make_list_value_pb(self.start_closed) | ||
| else: | ||
| kwargs['start_closed'] = _make_list_value_pb([]) |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
1b76a11 to
e181398
Compare
I need to clean this up a bit, but feel free to have a brief look in case I'm doing something atrocious.
I changed the behavior of a default constructor to include all instead of raising ValueError, which is the same behavior exhibited if you don't specify a end_key.
Also fixed a bug where if an end_key doesn't get specified, it adds the required field before it passes into pb.