Skip to content

Spanner: Make rows, consume_all and consume_next private #4492

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.

Sign up for GitHub

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

Jump to bottom
Merged
chemelnucfin merged 4 commits into from
Dec 13, 2017
Merged

Spanner: Make rows, consume_all and consume_next private #4492

chemelnucfin merged 4 commits into from
Dec 13, 2017

Conversation

@chemelnucfin
Copy link
Contributor

@chemelnucfin chemelnucfin commented Nov 29, 2017 •
edited
Loading

See #4414 and #4204

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

chemelnucfin commented Dec 6, 2017

@tseaver PTAL, thanks.

tseaver
tseaver reviewed Dec 6, 2017
View reviewed changes
spanner/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
spanner/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

spanner/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
Copy link
Contributor

@tseaver tseaver left a comment

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
spanner/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
spanner/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
Copy link
Contributor Author

chemelnucfin commented Dec 7, 2017

@vkedia @tseaver PTAL, thanks.

@chemelnucfin chemelnucfin mentioned this pull request Dec 8, 2017
Closed
tseaver
tseaver suggested changes Dec 13, 2017
View reviewed changes
spanner/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

spanner/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
Copy link
Contributor Author

chemelnucfin commented Dec 13, 2017

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

@chemelnucfin chemelnucfin mentioned this pull request Dec 13, 2017
Merged
@chemelnucfin chemelnucfin merged commit 460b606 into googleapis:master Dec 13, 2017
@chemelnucfin chemelnucfin deleted the spanner_rows_rename branch December 13, 2017 21:34
@chemelnucfin chemelnucfin mentioned this pull request Dec 15, 2017
Closed
@tseaver tseaver mentioned this pull request Jan 10, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lukesneeringer lukesneeringer Awaiting requested review from lukesneeringer

2 more reviewers

@vkedia vkedia vkedia left review comments

@tseaver tseaver tseaver approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

api: spanner Issues related to the Spanner API. cla: yes This human has signed the Contributor License Agreement. type: cleanup An 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