Fix overloads and remove PathLike in finders

[andersk]

I have made things!

Django really requires these paths to be str. For example, in FileSystemFinder, .find(path) calls .find_location(…, path, …) which evaluates path.startswith(prefix) and path[len(prefix) :]; these don’t work on arbitrary PathLike objects.

Related issues

• Refs update all path related operations to have more accurate types #713 (cc @terencehonles).

[sobolevn]

CC @terencehonles

[sobolevn]

Here's the function we are talking about: https://github.com/django/django/blob/3.2/django/contrib/staticfiles/finders.py#L110-L122

    def find_location(self, root, path, prefix=None):
        """
        Find a requested static file in a location and return the found
        absolute path (or ``None`` if no match).
        """
        if prefix:
            prefix = '%s%s' % (prefix, os.sep)
            if not path.startswith(prefix):
                return None
            path = path[len(prefix):]
        path = safe_join(root, path)
        if os.path.exists(path):
            return 

It looks like PathLike will work just fine unless prefix is passed. Am I wrong?

[andersk]

The find method does pass prefix to find_location.

[terencehonles]

This does look like I possibly overlooked that string interpolation (and that means it also wouldn't work with bytes). I was very likely trying to address Any and when looking over the code it looked like it worked for this wider set of types. Do you know why mypy didn't catch that .startswith(...) didn't work?

I'll test this branch against our codebase but we don't use paths that much, I just noticed that other places were using paths and I thought I had reviewed the changes close enough and figured the tests would catch any other issues.

[terencehonles]

Now that I'm looking at the code a little more, and happened to look at a particular line, I believe I was mainly trying to get find(..., all=True) to return a list instead of a single item. I just ran the PR's changes against our codebase and there are no issues with it from our standpoint so we'd not be opening any follow up PRs 😉

Still curious why mypy didn't point out that I made things too wide.

[andersk]

Do you know why mypy didn't catch that .startswith(...) didn't work?

Mypy isn’t being run against the code of Django itself. There’s no automated check that any of the typing claims in django-stubs reflect reality.

[terencehonles]

Do you know why mypy didn't catch that .startswith(...) didn't work?

Mypy isn’t being run against the code of Django itself. There’s no automated check that any of the typing claims in django-stubs reflect reality.

I thought it was, is it only the tests?

[andersk]

We type-check our own tests (django-stubs/tests). The upstream Django code and tests have no type annotations; there’s nothing there that could be checked.

[terencehonles]

I don't believe that's correct, the script in scripts/typecheck_tests.py is running mypy against Django's tests and checking against some allowed list of errors. I haven't double checked the setup of that, but I had thought it was running against Django's source not just Django's tests.

I'll look at seeing what it might take to have stubtest run against Django's source since I'm not completely sure why we're running mypy against the tests since where I first realized we were doing this was in https://github.com/typeddjango/djangorestframework-stubs and that was a bit brittle because we don't supply the stubs anywhere. Since this project is the stubs for Django it makes sense to run stubtest against it.

Running it quickly right now I'm seeing that there's some errors in the stub files and some declarations and not types are defined. This may be a result of a bug in stubgen, but it also may just have been that someone added a buggy stub that mypy will normally ignore the error, but stubtest is pickier since it's supposed to be testing stubs for validity.

[terencehonles]

Either way that explains why mypy wasn't catching this issue and this PR should be clear for merging and I can have a follow up PR if stubtest makes sense to add for the CI checks.

[andersk]

Ah, I see. But I don’t think it’d help much to try to expand that from the tests to the whole source. There’s no way to type-check upstream finders.py against our finders.pyi; mypy will only load one or the other. (See python/mypy#5028, which is unlikely to be worked on.)

[terencehonles]

Interesting, I didn't realize that didn't work. Thanks for pointing me at the other issue. I figured that would work.

I've played with retype before, and stubtest didn't seem to be doing what I wanted / producing too many errors for this project.

It does look like the only way to get what I am trying to do is to use retype to put the types back into place and then use mypy to check the types as I would have it expected it would have when being handed both the stubs and the source. I was having some issues with retype when I had played with it before and it's possible this all might be a bit futile at this point. I might have more time at a later date, but right now I've already poked at this more than I should have 😜.

[terencehonles]

Just to make it clear I'm cool with these changes 😉

[sobolevn]

Thanks!