bpo-19610: Warn if distutils is provided something other than a list to some fields by nascheme · Pull Request #4685

bpo-19610: Warn if distutils is provided something other than a list to some fields by nascheme · Pull Request #4685 · python/cpython

The commit of dcaed6b causes some 3rd party packages to fail to install, due to them passing a tuple for meta data fields. Rather than raise TypeError, generate a RuntimeWarning and convert the value using list().

https://bugs.python.org/issue19610

Choose a reason for hiding this comment

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

Needed to update Doc/distutils/apiref.rst too.

Choose a reason for hiding this comment

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

Would is better to use assertWarnsRegex() here and in other tests. Otherwise msg isn't used.

Choose a reason for hiding this comment

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

Would be better to move these checks outside of the with block. Otherwise it isn't clear what operation raises a RuntimeWarning.

Choose a reason for hiding this comment

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

It’s possible that the import was how it was because distutils is used to build CPython’s own extension modules. In doubt, let’s not change this.

Choose a reason for hiding this comment

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

Note that distutils also has its own PEP-282-style logging system, which may or may not easier to use here. The output is controlled by verbose/quiet options passed to setup.py rather than the interpreter warning configuration system, so testing with pip would be needed to make sure that package authors would see the messages by default.

Choose a reason for hiding this comment

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

The try/except dates back to Python 1.5 + 2.0 days. It was added because those versions didn't have 'warnings' and distutils was targeting those versions as well. I think removing it is safe but I will revert as it is a separate change from this issue.

Using the logging system sounds like a good idea. I'll look into that.

Choose a reason for hiding this comment

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

I have made the requested changes; please review again.

I tweaked the wording of the warning message, not sure it is the best.

Choose a reason for hiding this comment

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

Thanks for the patch. Tests look good; left comment about conditional use of warnings.

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

While we a here could you please add value = list(value) in set_requires() and set_obsoletes(). They already work with arbitrary iterables, but this can have unexpected effect in upload.py and register.py.

I have made the requested changes; please review again.

Thanks for making the requested changes!

@serhiy-storchaka, @merwok: please review the changes made to this pull request.

Choose a reason for hiding this comment

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

I thought you wanted to move this in a separate PR?

Choose a reason for hiding this comment

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

Is the prefix Warning redundant? Otherwise message seems good.

Choose a reason for hiding this comment

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

Another style nit: Can't we use {fieldname!r} instead of '{fieldname}'?

Choose a reason for hiding this comment

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

Minor: would you mind keeping two blank lines before and after this function?

Choose a reason for hiding this comment

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

This looks pretty good to me, thank you! I just left some comments on code style.

Choose a reason for hiding this comment

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

Style nit: Replacing \ with

from test.support import (
    TESTFN, captured_stdout, captured_stderr, run_unittest,
)

may be better.

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

Another style nit: Can't we use {fieldname!r} instead of '{fieldname}'?

Choose a reason for hiding this comment

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

Style nit: I think we can now inline this in line 395:

self.assertIn(msg, error.getvalue())

Introduce a helper function _ensure_list() that reduces code
duplication.