Skip to content

Spanner: Make rows, consume_all and consume_next private - #4492

Merged
chemelnucfin merged 4 commits into
googleapis:masterfrom
chemelnucfin:spanner_rows_rename
Dec 13, 2017
Merged

Spanner: Make rows, consume_all and consume_next private#4492
chemelnucfin merged 4 commits into
googleapis:masterfrom
chemelnucfin:spanner_rows_rename

Conversation

@chemelnucfin

@chemelnucfinchemelnucfin commented Nov 29, 2017•
edited
Loading

Copy link
Copy Markdown
Contributor

See #4414 and #4204

@googlebotgooglebot added the cla: yes This human has signed the Contributor License Agreement. label Nov 29, 2017
@chemelnucfinchemelnucfin added api: spanner Issues related to the Spanner API. type: cleanup An internal cleanup or hygiene concern. labels Nov 29, 2017
@chemelnucfin

chemelnucfin commented Dec 6, 2017

Copy link
Copy Markdown
ContributorAuthor

@tseaver PTAL, thanks.

tseaver
tseaver reviewed Dec 6, 2017
View reviewed changes
Comment threadspanner/google/cloud/spanner_v1/streamed.py Outdated
def __init__(self, response_iterator, source=None):
self._response_iterator = response_iterator
self._rows = [] # Fully-processed rows
self._processed_rows = [] # Fully-processed rows

This comment was marked as spam.

Sign in to view

tseaver
tseaver suggested changes Dec 6, 2017
View reviewed changes
Comment threadspanner/tests/system/test_system.py Outdated
while True:
try:
streamed.consume_next()
streamed._consume_next()

This comment was marked as spam.

Sign in to view

Comment threadspanner/tests/system/test_system.py Outdated
retrieved += len(streamed.rows)
streamed.rows[:] = ()
retrieved += len(streamed._rows)
streamed._rows[:] = ()

This comment was marked as spam.

Sign in to view

tseaver
tseaver suggested changes Dec 6, 2017
View reviewed changes

@tseavertseaver left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's just delete the two system tests: they are redundant with those in other testcases.

vkedia
vkedia reviewed Dec 7, 2017
View reviewed changes
Comment threadspanner/google/cloud/spanner_v1/streamed.py Outdated

@property
def rows(self):
def _rows(self):

This comment was marked as spam.

Sign in to view

vkedia
vkedia reviewed Dec 7, 2017
View reviewed changes
Comment threadspanner/google/cloud/spanner_v1/streamed.py Outdated
self._merge_values(values)

def consume_all(self):
def _consume_all(self):

This comment was marked as spam.

Sign in to view

@chemelnucfin

chemelnucfin commented Dec 7, 2017

Copy link
Copy Markdown
ContributorAuthor

@vkedia@tseaver PTAL, thanks.

@chemelnucfinchemelnucfin mentioned this pull request Dec 8, 2017
Closed
tseaver
tseaver suggested changes Dec 13, 2017
View reviewed changes
Comment threadspanner/tests/unit/test_snapshot.py Outdated
self.assertIsNone(result_set._source)

result_set._consume_all()
# result_set._consume_all()

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

Comment threadspanner/tests/unit/test_snapshot.py
# result_set._consume_all()

self.assertEqual(list(result_set._rows), VALUES)
self.assertEqual(list(result_set), VALUES)

This comment was marked as spam.

Sign in to view

@chemelnucfin

chemelnucfin commented Dec 13, 2017

Copy link
Copy Markdown
ContributorAuthor

I just changed the _rows to list. I will put that in another PR.

@chemelnucfinchemelnucfin mentioned this pull request Dec 13, 2017
Merged
@chemelnucfin
chemelnucfin merged commit 460b606 into googleapis:masterDec 13, 2017
@chemelnucfin
chemelnucfin deleted the spanner_rows_rename branch December 13, 2017 21:34
@chemelnucfinchemelnucfin mentioned this pull request Dec 15, 2017
Closed
@tseavertseaver mentioned this pull request Jan 10, 2018
Merged
parthea pushed a commit that referenced this pull request Mar 9, 2026
@chemelnucfin
Spanner: Make rows, consume_all and consume_next private (#4492)
8041ed3
* Spanner: Make rows, consume_all and consume_next private
* review changes
* Spanner: remove _consume_all and _rows methods
* Spanner: delete unnecessary tests
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lukesneeringerlukesneeringerAwaiting requested review from lukesneeringer
2 more reviewers
@vkediavkediavkedia left review comments
@tseavertseavertseaver approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

api: spannerIssues related to the Spanner API.cla: yesThis human has signed the Contributor License Agreement.type: cleanupAn internal cleanup or hygiene concern.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@chemelnucfin@tseaver@vkedia@googlebot