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

Remove '_implicit_environ' #979

Merged
merged 11 commits into from
Jul 13, 2015
Merged

Remove '_implicit_environ' #979

merged 11 commits into from
Jul 13, 2015

Conversation

tseaver
Copy link
Contributor

@tseaver tseaver commented Jul 10, 2015

Uses #978 as a base.

Final cleanup for #944.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jul 10, 2015
@tseaver tseaver mentioned this pull request Jul 10, 2015
Closed
8 tasks
@dhermes
Copy link
Contributor

dhermes commented Jul 10, 2015

Ping me here when #978 is in.

@tseaver
Copy link
Contributor Author

tseaver commented Jul 10, 2015

@dhermes I've rebased against the latest in #978. I will rebase again and ping you after it lands on master.

@tseaver
Copy link
Contributor Author

tseaver commented Jul 11, 2015

@dhermes now rebased against master, after merging #978.

@tseaver
Update batch/transaction/query to hold client.
ca42fdf 
Update batch/transaction to push themselves onto client's stack, rather
than batches.

Updated API-invoking methods to take 'client', rather than 'connection',
falling back to the client held by self.

See #944.
@tseaver
Remove 'gcloud.datastore.set_defaults'.
c16dae4 
Also remove symbols imported for conveniences from '_implicit_environ'.
@tseaver
Remove implicit dataset ID lookup from key.
94c9c11
@tseaver
Move dataset ID / connection discovery from '_implicit_environ' to 'c…
dd99e2c 
…lient'.
@tseaver
Remove spurious implicit environ testing support.
acde44c
@tseaver
Remove '_implicit_environ' and testing support.
f2c151a
@tseaver
Update system tests to use client in place of '_implicit_envioron.'
4a914db
@tseaver
Undo odd rebase artefacts.
1d9ca7d
@tseaver
Fix coverage lapses induced by merge.
e212d9b
@tseaver
Copy link
Contributor Author

tseaver commented Jul 11, 2015

@dhermes Rebased after merge of #978 and #982. PTAL

@tseaver
Copy link
Contributor Author

tseaver commented Jul 13, 2015

I'm not as fussy as you about the commit history -- if you'd like, I can just squash the whole PR into a single commit.

@dhermes
Copy link
Contributor

dhermes commented Jul 13, 2015

I'm not that fussy.

I think re-committing the "Update batch/transaction/query to hold client." commit only to remove those changes later is a bit silly, but not worth arguing over. (Just re-read my comment yesterday, noticed "moral" instead of "normal", unintentionally a bit dramatic by me! Reviews from phone will do that to you.) Your "git style" can lead to issues like that since you always add review changes as new commits. In this case (this PR and #978) you're better off using git cherry-pick on the upstream commits than git rebase.

Code LGTM. Feel free to merge.

tseaver added a commit that referenced this pull request Jul 13, 2015
@tseaver
Merge pull request #979 from tseaver/944-remove__implicit_environ
8373227 
Remove '_implicit_environ'
@tseaver tseaver merged commit 8373227 into googleapis:master Jul 13, 2015
@tseaver tseaver deleted the 944-remove__implicit_environ branch July 13, 2015 18:20
@dhermes dhermes mentioned this pull request Jul 21, 2015
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tseaver @dhermes @googlebot