The Wayback Machine - https://web.archive.org/web/20201020071614/https://github.com/google/python-fire/pull/135
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

Fix several warnings in the diff example #135

Merged
merged 2 commits into from Aug 28, 2018
Merged

Conversation

@BoboTiG
Copy link
Contributor

@BoboTiG BoboTiG commented Aug 18, 2018

Fixes ResourceWarning unclosed file:

examples/diff/diff.py:78: ResourceWarning: unclosed file <_io.TextIOWrapper name='/tmp/tmpnby8pemm' mode='U' encoding='UTF-8'>
    self.tolines = open(tofile, 'U').readlines()

And DeprecationWarning 'U' mode is deprecated:

examples/diff/diff_test.py::DiffTest::testUnifiedDiff
  examples/diff/diff.py:77: DeprecationWarning: 'U' mode is deprecated
    with open(fromfile, 'U') as f:
@googlebot
Copy link

@googlebot googlebot commented Aug 18, 2018

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers
@BoboTiG
Copy link
Contributor Author

@BoboTiG BoboTiG commented Aug 18, 2018

I signed it!

@googlebot
Copy link

@googlebot googlebot commented Aug 18, 2018

CLAs look good, thanks!

@dbieber
Copy link
Member

@dbieber dbieber commented Aug 18, 2018

Thanks for the PR, I’ll take a look this week

@dbieber
Copy link
Member

@dbieber dbieber commented Aug 20, 2018

Thanks for fixing those warnings, this looks good.
For context, it's written the way it is because this is a direct translation of https://docs.python.org/2/library/difflib.html (section 7.4.5) to Fire.
I wonder if there's a way to push the changes upstream to the Python 2 documentation so we don't diverge from their example.

The context manager change lgtm.
For the open mode change, is there a way to do this without checking the version number with sys? E.g. does six have this mode? Or should we just use mode 'r' in all cases? I ask because putting the version check directly in the example sets us up for needing future changes later.

@BoboTiG
Copy link
Contributor Author

@BoboTiG BoboTiG commented Aug 20, 2018

Thanks for the review.

I will open a PR for the Python documentation, good idea.
About the open mode, I would say we can go without any open mode as r is the default one. I am not aware a such a constant in six.

@dbieber
Copy link
Member

@dbieber dbieber commented Aug 20, 2018

sgtm, let's remove the open mode.

I will open a PR for the Python documentation, good idea.

Great, thanks. cc me, or drop a link to the PR in this issue once it's open?

@BoboTiG
Copy link
Contributor Author

@BoboTiG BoboTiG commented Aug 20, 2018

Sure :)

@dbieber
Copy link
Member

@dbieber dbieber commented Aug 24, 2018

checking in on removing the open mode

@BoboTiG
Copy link
Contributor Author

@BoboTiG BoboTiG commented Aug 24, 2018

@dbieber
Copy link
Member

@dbieber dbieber commented Aug 24, 2018

Okay Sounds good

@BoboTiG
Copy link
Contributor Author

@BoboTiG BoboTiG commented Aug 25, 2018

I aligned the script with the Python 3 version. WDYT?
You may have received notifications, but this is the BPO: https://bugs.python.org/issue34500.

@BoboTiG BoboTiG changed the title Fix several warnings in the diff example [2.7] Fix several warnings in the diff example Aug 25, 2018
@BoboTiG BoboTiG changed the title [2.7] Fix several warnings in the diff example Fix several warnings in the diff example Aug 25, 2018
@dbieber
Copy link
Member

@dbieber dbieber commented Aug 25, 2018

lgtm,
will squash and merge this coming week.

@dbieber dbieber merged commit 54f91a2 into google:master Aug 28, 2018
2 checks passed
2 checks passed
cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@dbieber
Copy link
Member

@dbieber dbieber commented Aug 29, 2018

All set, thanks!

@BoboTiG BoboTiG deleted the BoboTiG:fix-several-warnings branch Aug 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
You can’t perform that action at this time.