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.

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

chore: sync v3 with main branch #822

Closed
wants to merge 5 commits into from
Closed

chore: sync v3 with main branch #822

wants to merge 5 commits into from

Conversation

tswast
Copy link
Contributor

@tswast tswast commented Jul 27, 2021

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

Fixes #<issue_number_goes_here> 🦕

tswast and others added 4 commits July 27, 2021 13:04
@tswast
chore: protect v3.x.x branch (#816)
3b70891 
* chore: protect v3.x.x branch

In preparation for breaking changes.

* force pattern to be a string

* simplify branch name
@plamut
fix: no longer raise a warning in to_dataframe if max_results set (…
3c1be14 
…#815)

That warning should only be used when BQ Storage client is
explicitly passed in to RowIterator methods when max_results
value is also set.
@gcf-owl-bot
feat: Update proto definitions for bigquery/v2 to support new proto f…
fe7a902 
…ields for BQML. (#817)

PiperOrigin-RevId: 387137741

Source-Link: googleapis/googleapis@8962c92

Source-Link: googleapis/googleapis-gen@102f1b4
@release-please
chore: release 2.23.0 (#819)
02bbdae 
Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
@tswast tswast requested a review from a team July 27, 2021 21:25
@tswast tswast requested a review from a team as a code owner July 27, 2021 21:25
@tswast tswast requested review from loferris and removed request for a team July 27, 2021 21:25
@product-auto-label product-auto-label bot added the api: bigquery Issues related to the googleapis/python-bigquery API. label Jul 27, 2021
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Jul 27, 2021
@tswast
Copy link
Contributor Author

tswast commented Jul 27, 2021

I wonder if squashing is going to make resolving commits harder in the future? Ugh.

@tswast tswast added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Jul 27, 2021
@tswast
Copy link
Contributor Author

tswast commented Jul 27, 2021

Maybe we keep this PR open and periodically add merge commits so that we can resolve conflicts incrementally?

@tswast tswast marked this pull request as draft July 27, 2021 21:49
@tseaver
Copy link
Contributor

tseaver commented Jul 27, 2021

Except for the fact that you just added branch protection to v3, it would be simpler to just rebase the v3 PR against current master periodically and force-push it.

@plamut
Copy link
Contributor

plamut commented Jul 28, 2021

+1 for merging/rebasing master changes periodically, since the merge conflicts will be smaller and easier to handle.

@plamut
Copy link
Contributor

plamut commented Jul 28, 2021 •
edited
Loading

Also, I'm not entirely comfortable with syncing changes from master without a prior review, since (semantically) incompatible changes can sneak in without warning in the form of a merge conflict (comment).

With that in mind it might actually be better to bring in the changes from master through merges in a from of PRs that need to be reviewed first (like this one). I'll open one such PR shortly.

Update: created. Was faster than tinkering with Tim's PR branch and to first send my changes there.

plamut
plamut reviewed Jul 28, 2021
View reviewed changes
tests/unit/test_table.py
Comment on lines +2483 to +2486
@unittest.skipIf(
bigquery_storage is None, "Requires `google-cloud-bigquery-storage`"
)
@unittest.skipIf(pyarrow is None, "Requires `pyarrow`")
Copy link
Contributor

Choose a reason for hiding this comment

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

This is what I was talking about, i.e. "semantic conflicts". We should be careful to not let these through, since they do not necessarily create loud merge conflicts.

It can get tricky :/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch!

@plamut plamut mentioned this pull request Jul 28, 2021
Merged
4 tasks
@plamut
Copy link
Contributor

plamut commented Jul 28, 2021

Maybe we keep this PR open and periodically add merge commits so that we can resolve conflicts incrementally?

After resolving several semantic conflicts myself in the related PR, I think it will be easier to not keep a PR open for too long, but instead incrementally accumulate the changes from master in the v3 branch. This will allow any maintainer to open (smaller) PRs against the common v3 branch, say, after their new non-trivial contributions have been merged into master.

For example, I knew I had to pay attention to redundant skipIf decorators, because it was my code, but might have overlooked semantic conflicts in other people's contributions. The authors probably know best how to bring their changes to the v3 branch.

@tswast tswast closed this Jul 28, 2021
@tswast tswast deleted the tswast-master branch July 29, 2021 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@plamut plamut plamut left review comments

@loferris loferris Awaiting requested review from loferris

Assignees
No one assigned
Labels
api: bigquery Issues related to the googleapis/python-bigquery 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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tswast @tseaver @plamut