Skip to content
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

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

Closed
wants to merge 1 commit into from

Conversation

dhermes
Copy link
Contributor

@dhermes dhermes commented Feb 29, 2016

Fixes #1548.

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

dhermes commented Feb 29, 2016

@jgeewax @jonparrott LMK what you guys think of this

@theacodes
Copy link
Contributor

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

@dhermes
Copy link
Contributor Author

dhermes commented Feb 29, 2016

👍

@dhermes
Copy link
Contributor Author

dhermes commented Feb 29, 2016

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

@tseaver
Copy link
Contributor

tseaver commented Feb 29, 2016

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.

@theacodes
Copy link
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
Copy link
Contributor Author

dhermes commented Feb 29, 2016

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

dhermes commented Mar 1, 2016

@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
Copy link
Contributor

tseaver commented Mar 1, 2016

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

@theacodes
Copy link
Contributor

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

Agreed.

@dhermes
Copy link
Contributor Author

dhermes commented Mar 1, 2016

Working on it now.

@dhermes 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
Copy link
Contributor Author

dhermes commented Mar 11, 2016

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

@dhermes dhermes closed this Mar 11, 2016
@dhermes dhermes deleted the fix-1548 branch March 11, 2016 20:42
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