files property should never be None

[jaraco]

In GitLab by @JohannesBuchner on Jun 11, 2019, 15:59

Hi,

I noticed pytest has an issue interpreting the files property being None. I wonder if you would accept the following patch:

diff --git a/importlib_metadata/__init__.py b/importlib_metadata/__init__.py
index 1d0a1bd..e12a3ce 100644
--- a/importlib_metadata/__init__.py
+++ b/importlib_metadata/__init__.py
@@ -240,7 +240,7 @@ class Distribution:
             result.dist = self
             return result
 
-        return file_lines and starmap(make_file, csv.reader(file_lines))
+        return starmap(make_file, csv.reader(file_lines)) if file_lines else []
 
     def _read_files_distinfo(self):
         """

A list seems much more natural for the files property.

Cheers,
Johannes

[jaraco]

In GitLab by @warsaw on Jun 11, 2019, 16:25

Did this happen to you? Can you provide a test case?

[jaraco]

In GitLab by @jaraco on Jun 11, 2019, 18:38

Right now, a None result indicates that no files metadata was found whereas [] represents that a file was found with no entries in it. That seems like a useful distinction to make. Caller can always use files() or [] or more_itertools.always_iterable(files()) if the null result is meant to be indistinguishable from the empty result.

Can you explain more about how this happened and why you expect the result to always be a list?

[jaraco]

In GitLab by @JohannesBuchner on Jun 11, 2019, 19:34

pytest got the use wrong and now I have a broken pytest installation on my system (they haven't issued a fix yet, but merged a patch into master).

pytest-dev/pytest#5434
pytest-dev/pytest#5389

[jaraco]

In GitLab by @warsaw on Jun 11, 2019, 23:39

@jaraco Let's be sure these return value semantics are fully documented.

[jaraco]

In GitLab by @jaraco on Jun 12, 2019, 14:14

Agreed it would be worthwhile to be explicit about null/error/exceptional conditions. The tests and docs currently focus on illustrating behavior only for the nominal cases.

[jaraco]

In GitLab by @jaraco on Jun 12, 2019, 14:19

I'd like to know more about the conditions in which the pytest installation broke. I would only have expected that error to emerge if there was some package without a file record. Isn't it reasonable for .files to raise an exception if no files are available to report? Are you able to replicate the conditions that led to the error? I think it would be important to analyze those conditions to inform what a more correct response might be.

[jaraco]

In GitLab by @JohannesBuchner on Jun 12, 2019, 14:36

I was able to trace it to pytest trying to find pytest11-group packages. With some debug output in _pytest.config before the error caused by the loop::

        for dist in importlib_metadata.distributions():
            if any(ep.group == "pytest11" for ep in dist.entry_points):
                print("dist:", dist.metadata)
                print("    path:", dist._path)
                #print("    text:", dist.read_text)
                print("    ", dir(dist))
                print("    files:", dist.files)

        package_files = (
            str(file)
            for dist in importlib_metadata.distributions()
            if any(ep.group == "pytest11" for ep in dist.entry_points)
            for file in dist.files
        )

I get the following package information before the "TypeError: 'NoneType' object is not iterable" error:

dist: Metadata-Version: 1.2
Name: hypothesis
Version: 3.44.1
Summary: A library for property based testing
Home-page: https://github.com/HypothesisWorks/hypothesis-python
Author: David R. MacIver
Author-email: [email protected]
License: MPL v2
Description-Content-Type: UNKNOWN
Description: ==========
        Hypothesis
        ==========        
        Hypothesis is an advanced testing [blabla ... ]
Platform: UNKNOWN
Classifier: Development Status :: 5 - Production/Stable
Classifier: Intended Audience :: Developers
Classifier: License :: OSI Approved :: Mozilla Public License 2.0 (MPL 2.0)
Classifier: Operating System :: Unix
Classifier: Operating System :: POSIX
Classifier: Operating System :: Microsoft :: Windows
Classifier: Programming Language :: Python
Classifier: Programming Language :: Python :: 2.7
Classifier: Programming Language :: Python :: 3
Classifier: Programming Language :: Python :: 3.4
Classifier: Programming Language :: Python :: 3.5
Classifier: Programming Language :: Python :: 3.6
Classifier: Programming Language :: Python :: Implementation :: CPython
Classifier: Programming Language :: Python :: Implementation :: PyPy
Classifier: Topic :: Software Development :: Testing
Requires-Python: >=2.7, !=3.0.*, !=3.1.*, !=3.2.*, !=3.3.*


    path: /usr/lib/python3/dist-packages/hypothesis-3.44.1.egg-info

I packaged that folder up for you: hypothesis-egg.tar.gz

[jaraco]

In GitLab by @JohannesBuchner on Jun 12, 2019, 14:45

Right now, a None result indicates that no files metadata was found whereas [] represents that a file was found with no entries in it. That seems like a useful distinction to make.

Could you say when this distinction would/could be useful?

No files metadata sounds like one way of expressing zero files are here.

[jaraco]

In GitLab by @jaraco on Sep 1, 2019, 16:40

So I've downloaded that metadata (thanks for providing that) and I can see it doesn't include any metadata about which files are present.

$ http https://gitlab.com/python-devs/importlib_metadata/uploads/dcc93b2c87bb83bc08eb8d1afd808596/hypothesis-egg.tar.gz | tar xz
$ ls hyp*.egg-info                                                                                   
PKG-INFO             dependency_links.txt entry_points.txt     not-zip-safe         requires.txt         top_level.txt

Since SOURCES.txt doesn't exist, the result of distribution('hypothesis').files will be None by design, and pytest should use one of the aforementioned techniques to handle that condition (assuming this use-case is a supported use-case).

Let's be sure these return value semantics are fully documented.

Agreed. I'll now proceed to expand on the documentation.

[jaraco]

In GitLab by @jaraco on Sep 1, 2019, 16:57

mentioned in commit 1af2197

[jaraco]

In GitLab by @jaraco on Sep 1, 2019, 16:57

mentioned in commit 753d4ef

[jaraco]

In GitLab by @jaraco on Sep 1, 2019, 17:00

I'm wondering if the interface should be less protective and should instead always throw exceptions when read_text attempts to read a file that doesn't exist... and leave it up to the caller to trap such exceptions. That would seem to me to be more Pythonic, but would be unfriendly to the pytest use case described above and probably to other use-cases as well.

[jaraco]

In GitLab by @jaraco on Sep 2, 2019, 12:20

closed via commit 1af2197

[jaraco]

In GitLab by @jaraco on Sep 2, 2019, 12:20

closed via commit f5939bf

[jaraco]

In GitLab by @jaraco on Sep 2, 2019, 14:32

mentioned in commit f5939bf

[jaraco]

In GitLab by @warsaw on Sep 2, 2019, 16:37

I agree it feels more Pythonic, but if it's less consumer friendly, then practicality beats purity.