Skip to content

Revert "Spanner: Make sure **exactly** one of start_*/end_* are p… - #4733

Merged
theacodes merged 2 commits into
masterfrom
revert-4618
Jan 11, 2018
Merged

Revert "Spanner: Make sure **exactly** one of start_*/end_* are p…#4733
theacodes merged 2 commits into
masterfrom
revert-4618

Conversation

@theacodes

@theacodestheacodes commented Jan 10, 2018•
edited
Loading

Copy link
Copy Markdown
Contributor

…assed to KeyRange (#4618)"

This reverts commit 4d6cd26.

Towards #4694

@googlebotgooglebot added the cla: yes This human has signed the Contributor License Agreement. label Jan 10, 2018
@dhermes

dhermes commented Jan 10, 2018

Copy link
Copy Markdown
Contributor

This LGTM but I don't think you should close #4694. Or if you do, you should re-open the issue that #4618 seeked to fix.

@theacodes

theacodes commented Jan 10, 2018

Copy link
Copy Markdown
ContributorAuthor

Fair enough, will leave it open.

@theacodes

theacodes commented Jan 11, 2018

Copy link
Copy Markdown
ContributorAuthor

@tseaver can you shed some light on the failure here: https://circleci.com/gh/GoogleCloudPlatform/google-cloud-python/5141?

@theacodes

theacodes commented Jan 11, 2018

Copy link
Copy Markdown
ContributorAuthor

nvm, @dhermes helped me figure it out.

@dhermes

dhermes commented Jan 11, 2018

Copy link
Copy Markdown
Contributor

And Chuck Norris!

@theacodes

theacodes commented Jan 11, 2018

Copy link
Copy Markdown
ContributorAuthor

Is that linked in the contributing guide? If not, it should be. P0.

@theacodes

theacodes commented Jan 11, 2018

Copy link
Copy Markdown
ContributorAuthor

Ignoring conflict error as it's just due to noise from another spanner system tests (I will not release this without a green build on the release PR).

@theacodes
theacodes merged commit 36c7a16 into masterJan 11, 2018
@theacodes
theacodes deleted the revert-4618 branch January 11, 2018 00:34
@chemelnucfin

chemelnucfin commented Jan 11, 2018 via email •
edited by dhermes
Loading

Copy link
Copy Markdown
Contributor

@tseaver

tseaver commented Jan 11, 2018

Copy link
Copy Markdown
Contributor

Rather than raising a ValueError if the caller passes a start_open / start_closed but neither end_open / end_closed, the constructor should just assign the empty list to end_open (this was the documented workaround for the caller in #4618: we can just do it on the caller's behalf).

Likewise for the case where the caller passes an end_open / end_closed but neither of start_open / start_closed: the constructor should just assign start_open = [].

@chemelnucfin

chemelnucfin commented Jan 11, 2018

Copy link
Copy Markdown
Contributor

For the first case, I think assigning it to end_closed is a better choice, as I believe that end_open excludes everything.

@chemelnucfin

chemelnucfin commented Jan 11, 2018

Copy link
Copy Markdown
Contributor

Likewise for the end_closed. Should I make the change?

@theacodes

theacodes commented Jan 11, 2018

Copy link
Copy Markdown
ContributorAuthor

Go for it, if you're faster than CI I'll rebase it into #4732.

parthea pushed a commit that referenced this pull request Mar 9, 2026
Jon Wayne Parrott
Revert "Spanner: Make sure **exactly** one of start_*/end_* are p… (
bd6a255
#4733)
Revert "Spanner: Make sure **exactly** one of `start_*`/`end_*` are passed to KeyRange (#4618)"
This reverts commit 73be2884cffaf33de8d652e5abdf801cac25888d.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lukesneeringerlukesneeringerAwaiting requested review from lukesneeringer
1 more reviewer
@tseavertseavertseaver approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla: yesThis human has signed the Contributor License Agreement.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@theacodes@dhermes@chemelnucfin@tseaver@googlebot