Skip to content

Compare files manually. #710

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

Merged
merged 1 commit into from
Dec 13, 2016
Merged

Compare files manually. #710

merged 1 commit into from
Dec 13, 2016

Conversation

jerjou
Copy link
Contributor

@jerjou jerjou commented Dec 9, 2016

Addresses #707

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Dec 9, 2016
@jerjou jerjou assigned jerjou, theacodes and ryanmats and unassigned jerjou Dec 9, 2016
@theacodes
Copy link
Contributor

Hmm, I liked the idea of just closing the handle to the named temporary file and then using filecmp.

@jerjou
Copy link
Contributor Author

jerjou commented Dec 9, 2016

Oh. It just seemed less clean. eg:

  • You're now manually in charge of file cleanup, which feels like it should be the context manager's job
  • The use of filecmp with a filename when you've already got a handle on the file always felt a bit hacky.
  • The file comparison is now both inside and outside of the context manager, which breaks it up visually even though it's the same conceptual operation

Granted, this is a bit more verbose and low-level, but I feel like it flows better.

@theacodes
Copy link
Contributor

Can we just remove the file comparison altogether from this sample?

@jerjou jerjou force-pushed the storage branch 2 times, most recently from 382ef96 to 249a2f2 Compare December 12, 2016 19:10
@jerjou
Copy link
Contributor Author

jerjou commented Dec 12, 2016

Oh yeah - sure. Apparently this file isn't even referenced in the docs anymore.
Done.

import json
import tempfile

from googleapiclient import discovery
from googleapiclient import http

from oauth2client.client import GoogleCredentials
from six.moves import zip_longest
Copy link
Contributor

Choose a reason for hiding this comment

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

remove this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah. right.

@ryanmats ryanmats assigned theacodes and ryanmats and unassigned theacodes and ryanmats Dec 13, 2016
@theacodes theacodes merged commit ca2d5fc into master Dec 13, 2016
@theacodes theacodes deleted the storage branch December 13, 2016 18:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants