Skip to content

Only produce python3 warning with python3 warnings enabled#24

Merged
jaraco merged 1 commit intojaraco:masterfrom
asottile:patch-2
Jan 25, 2019
Merged

Only produce python3 warning with python3 warnings enabled#24
jaraco merged 1 commit intojaraco:masterfrom
asottile:patch-2

Conversation

@asottile
Copy link
Copy Markdown
Contributor

As it is, this warning is quite difficult to avoid in a number of circumstances that would automatically be fixed in python3.x. As such, this warning is often not useful. Here’s a few examples where I’ve encountered this that are a bit annoying to fix:

  • flake8: takes commandline arguments (sys.argv is native strings (bytes py2, str py3)) which are passed to configparser
  • flake8: passes string literals to configparser (string literals are native strings)
  • entrypoints: passes os.path.join(sys.path entry, …) to configparser (sys.path is native strings)

These warnings only appear in python2.x and any sane library is already testing against python3.x where these would be errors.

As such, I propose either outright removing this warning, or (as this PR does) guarding it by sys.py3kwarning which is only True when running with python2 -3 ...

As it is, this warning is quite difficult to avoid in a number of circumstances that would automatically be fixed in python3.x. As such, this warning is often not useful. Here’s a few examples where I’ve encountered this that are a bit annoying to fix:

- flake8: takes commandline arguments (sys.argv is native strings (bytes py2, str py3)) which are passed to configparser
- flake8: passes string literals to configparser (string literals are native strings)
- entrypoints: passes os.path.join(sys.path entry, …) to configparser (sys.path is native strings)

‌

These warnings only appear in python2.x and any sane library is already testing against python3.x where these would be errors.

‌

As such, I propose either outright removing this warning, or (as this PR does) guarding it by `sys.py3kwarning` which is only `True` when running with `python2 -3 ...`
@jaraco jaraco merged commit c554de3 into jaraco:master Jan 25, 2019
Return list of successfully read files.
"""
if PY2 and isinstance(filenames, bytes):
if PY2 and isinstance(filenames, bytes) and sys.py3kwarning:
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This additional guard should have been added inside the if, only for the warnings.warn().
By adding it here, filenames = [filenames] is not executed.

This is breaking flake8 (which depends on configparser under Python 2) for me, setup.cfg of my project seems to be ignored with this change, while it works normally if I remove it.

I see that this code has just been removed in the master branch, are you planning to release configparser 3.7.0 soon?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Which causes #27

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

are you planning to release configparser 3.7.0 soon?

Yes. I actually thought I had. Indeed, I'd tagged it, just not pushed the releases. I'll do that now.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Thanks! Both 3.5.3 and 3.7.0 fix the issue for me.

@asottile asottile deleted the patch-2 branch January 25, 2019 15:22
@asottile
Copy link
Copy Markdown
Contributor Author

Apologies on this, I should have been more careful :(

Thanks for fixing this!

@jaraco
Copy link
Copy Markdown
Owner

jaraco commented Jan 26, 2019

I too could have been more careful. I put too much trust in the test suite and the age of the (original) pull request. I found a broader lesson in the whole ordeal, though, which you may appreciate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants