FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Moving Bigtable Row.commit_modifications() into commit(). by dhermes · Pull Request #1550 · googleapis/google-cloud-python · GitHub

Moving Bigtable Row.commit_modifications() into commit(). - #1550

Closed
dhermes wants to merge 1 commit into
googleapis:masterfrom
dhermes:fix-1548
Closed

Moving Bigtable Row.commit_modifications() into commit().#1550
dhermes wants to merge 1 commit into
googleapis:masterfrom
dhermes:fix-1548

Conversation

dhermes commented Feb 29, 2016

Copy link
Copy Markdown
Contributor

Fixes #1548.

dhermes added the api: bigtable Issues related to the Bigtable API. label Feb 29, 2016
googlebot added the cla: yes This human has signed the Contributor License Agreement. label Feb 29, 2016

dhermes commented Feb 29, 2016

Copy link
Copy Markdown
Contributor Author

@jgeewax @jonparrott LMK what you guys think of this

Copy link
Copy Markdown
Contributor

This seems pretty reasonable in terms of the API surface/usage.

dhermes commented Feb 29, 2016

Copy link
Copy Markdown
Contributor Author

👍

dhermes commented Feb 29, 2016

Copy link
Copy Markdown
Contributor Author

I think I should update bigtable-data-api.rst as well

tseaver commented Feb 29, 2016

Copy link
Copy Markdown
Contributor

Now that I see the change, it feels like there should really be two different row classes in play, rather than having methods which are present, but only available based on the value of append.

Copy link
Copy Markdown
Contributor

Now that I see the change, it feels like there should really be two different row classes in play, rather than having methods which are present, but only available based on the value of append.

I'm somewhat leaning towards this as well. Row seems to have significant variadic behavior based on one argument.

dhermes commented Feb 29, 2016

Copy link
Copy Markdown
Contributor Author

Two classes or three? There are three distinct cases here. Also, do users want three?

The worst / grossest "variadic"-ness to me is in the output of commit().

dhermes commented Mar 1, 2016

Copy link
Copy Markdown
Contributor Author

@tseaver @jonparrott Bump (want to get a release out ASAP so this PR can't block too long).

Two questions:

  • Two or three row classes? (3 implicit types, "direct", "conditional" and "append")
  • Would users blanch at having to use 3 separate classes? Could we sufficiently ease this pain with the Table.row() factory?

Also @jgeewax Please weigh in.

tseaver commented Mar 1, 2016

Copy link
Copy Markdown
Contributor

Three classes, I think: the Table.row() factory could figure out which one to instantiate, based on the params passed?

Copy link
Copy Markdown
Contributor

the Table.row() factory could figure out which one to instantiate, based on the params passed?

Agreed.

dhermes commented Mar 1, 2016

Copy link
Copy Markdown
Contributor Author

Working on it now.

dhermes added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Mar 3, 2016

dhermes commented Mar 11, 2016

Copy link
Copy Markdown
Contributor Author

This can definitely be dropped since #1557 and others covered it.

dhermes closed this Mar 11, 2016
dhermes deleted the fix-1548 branch March 11, 2016 20:42
parthea pushed a commit that referenced this pull request Mar 9, 2026
feat(zb-experimental): Add AsyncMultiRangeDownloader and it's init method.
parthea pushed a commit that referenced this pull request Apr 1, 2026
* feat: support bq connection path format

For example, now a user specified bq connection like
"projects/project_id/locations/northamerica-northeast1/connections/conn-name"
would also be supported.

* include path format in tests with connection mismatch

* pass cloud_function_service_account="default" in more tests
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api: bigtable Issues related to the Bigtable API. cla: yes This human has signed the Contributor License Agreement. do not merge Indicates a pull request not ready for merge, due to either quality or timing.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants


Back | FazBrowse Home | New Git URL