bpo-19610: Warn if distutils is provided something other than a list to some fields

[nascheme]

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

[serhiy-storchaka]

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

[serhiy-storchaka]

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

[serhiy-storchaka]

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

[merwok]

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.

[merwok]

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.

[nascheme]

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.

[nascheme]

I have made the requested changes; please review again.

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

[merwok]

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

[bedevere-bot]

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

[serhiy-storchaka]

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.

[nascheme]

I have made the requested changes; please review again.

[bedevere-bot]

Thanks for making the requested changes!

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

[merwok]

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

[merwok]

Is the prefix Warning redundant? Otherwise message seems good.

[berkerpeksag]

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

[merwok]

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

[berkerpeksag]

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

[berkerpeksag]

Style nit: Replacing \ with

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

may be better.

[berkerpeksag]

Should we add tests for the requires and obsolotes fields similar to what you already added for the classifiers field?:

attrs = {...,
         'classifiers': ('Programming Language :: Python :: 3',)}
self.assertIsInstance(d.metadata.classifiers, list)
self.assertEqual(d.metadata.classifiers,
                 list(attrs['classifiers']))

If I don't miss something, the line list(value) is not covered by the current tests:

https://github.com/python/cpython/blob/bce095d9b0595a528a95bae3f764ee195a0c0d63/Lib/distutils/tests/test_dist.py#L301-L315
https://github.com/python/cpython/blob/bce095d9b0595a528a95bae3f764ee195a0c0d63/Lib/distutils/tests/test_dist.py#L323-L337

[berkerpeksag]

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

[berkerpeksag]

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

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