Skip to content

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.

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
Linchin added a commit that referenced this pull request Aug 18, 2025
* chore(python): Add Python 3.12

Source-Link: googleapis/synthtool@af16e6d
Post-Processor: gcr.io/cloud-devrel-public-resources/owlbot-python:latest@sha256:bacc3af03bff793a03add584537b36b5644342931ad989e3ba1171d3bd5399f5

* Add python 3.12 to setup.py

* add dependency for 3.12

* 🦉 Updates from OwlBot post-processor

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

* update owlbot

* 🦉 Updates from OwlBot post-processor

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

* 🦉 Updates from OwlBot post-processor

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

* undo owlbot change

* 🦉 Updates from OwlBot post-processor

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

---------

Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
Co-authored-by: Anthonios Partheniou <[email protected]>
Co-authored-by: Linchin <[email protected]>
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