Usage of check_file to display which pattern failed

[henryiii]

I'd like to add the failing pattern to my debug logging. I tried this:

    if (c := include_spec.check_file(p)).include:
        logger.debug(
            "Including {} {} because it is explicitly included by rule {!r}.",
            ptype,
            p,
            include_spec.patterns[c.index].pattern,
        )
        return True

But I've run into two or three typing issues.

1. Pattern has no attribute .pattern; this could be solved if the include spec was made Generic so that it could know it has a subclass, but is there a better way to print out an arbitrary Pattern? __str__() is not overloaded.
2. c.index could be None, so I get a complaint indexing patterns with it, except it can't be None if .include is True, I believe, so there might be a way to use a type guard or something here to indicate that these two types are linked.
3. .patterns is a Sequence (instead of a Collection from Python 3.6+), so you can't index it. It would be much more expensive for me to do list(include_spec.patterns)[c.index], since I know it's a list, but maybe this should be typed differently? Is there a reason to only allow Sequence here, vs. allowing random access?

Those issues make me think maybe I'm not approaching this correctly; upon finding check_file as a replacement for match_file, I was pretty sure that's what this was for, but maybe there's a better way?

[cpburnz]

1. Pattern has no attribute .pattern; this could be solved if the include spec was made Generic so that it could know it has a subclass, but is there a better way to print out an arbitrary Pattern? str() is not overloaded.

My original intention was for Pattern to be the abstract class to allow different types of pattern implementations other than regular expressions. I never ended up needing other types or received any requests, so regular expressions remain the only direct subclass.

Making PathSpec use Generic sounds like a good idea to allow specifying the Pattern sub type. It looks like 3.13 allows setting a default type, so I could set the default to Pattern, and RegexPattern can be used for more useful typing... really RegexPattern would be a more useful default, but that breaks the typing contract.

Implementing __str__ (or __repr__) would be useful for debugging.

2. c.index could be None, so I get a complaint indexing patterns with it, except it can't be None if .include is True, I believe, so there might be a way to use a type guard or something here to indicate that these two types are linked.

They are linked. .include and .index are either both None or not None. I think it would be possible to make a type guard for effectively CheckResultMatch and CheckResultNone.

3. .patterns is a Sequence (instead of a Collection from Python 3.6+), so you can't index it. It would be much more expensive for me to do list(include_spec.patterns)[c.index], since I know it's a list, but maybe this should be typed differently? Is there a reason to only allow Sequence here, vs. allowing random access?

You have those backward. A sequence supports indexing and random access. A collection does not (abstract container that's a parent to both list and set). include_spec.patterns[c.index] is valid.

Those issues make me think maybe I'm not approaching this correctly; upon finding check_file as a replacement for match_file, I was pretty sure that's what this was for, but maybe there's a better way?

You're approaching this the right way. You just found an area that could use improvement.

[henryiii]

You have those backward.

Ahh, you are correct; yes, I do. This is the only one that mypy wasn't complaining about, but my editor was complaining. Not sure why now, maybe one of the other two issues tripped it up earlier and made it give the "indexing not supported" message.

I think it would be possible to make a type guard

It's possible this is only something that would work with PEP 827, I'm not sure. I don't think I've had to set up a relationship like this before. I'm curious now.

Implementing __str__ (or __repr__) would be useful for debugging.

I think the current __repr__ is okay (though IMO it could be a little better), but I want to print out just the pattern, so that would be more likely to be a __str__ for a RegexPattern. Basically, I think this would be nice:

>>> p
GitIgnoreSpecPattern(pattern='!c')
>>> print(p)
!c

Though not critical, happy to use pattern for now.

setting a default type

I was really excited about this, but in practice default types are a bit less useful than I expected. When I made boost-histogram's Histogram Generic, I was pretty sure that Histogram[Double] would make sense as the default, but that caused Double to be assumed in cases when Histogram[Any] was correct when you just wrote Histogram, so it ended up being better without the default. I haven't looked at the details here.

Quick question: you have pytest>=9, but python_requires>=3.9; pytest 9 requires 3.10+ so I don't see how that is supposed to resolve? I'm just now starting to drop 3.8 from scikit-build-core (and packaging), so would like it if 3.9 stuck around a little longer, but there's a collision there (I can't use uv run even fro 3.14 due to the inability to solve a complete environment).

[cpburnz]

I think the current repr is okay (though IMO it could be a little better), but I want to print out just the pattern, so that would be more likely to be a str for a RegexPattern. Basically, I think this would be nice:

That sounds reasonable.

I was really excited about this, but in practice default types are a bit less useful than I expected. When I made boost-histogram's Histogram Generic, I was pretty sure that Histogram[Double] would make sense as the default, but that caused Double to be assumed in cases when Histogram[Any] was correct when you just wrote Histogram, so it ended up being better without the default. I haven't looked at the details here.

So when using a default type, explicitly using Histogram[Any] with Any is the same as just Histogram and would default to Histogram[Double]? I can see why it would be implemented that way because it's probably easier, but that's very limiting.

I think that limitation would be fine for PathSpec though. Every type would be expected to be a subclass of Pattern, so there's no need to support Any like with a graphing or numeric library.

Quick question: you have pytest>=9, but python_requires>=3.9; pytest 9 requires 3.10+ so I don't see how that is supposed to resolve? I'm just now starting to drop 3.8 from scikit-build-core (and packaging), so would like it if 3.9 stuck around a little longer, but there's a collision there (I can't use uv run even fro 3.14 due to the inability to solve a complete environment).

I have that mislabeled in pyproject.toml. The "tests" optional dependencies are for the benchmarks/ code only (I'll rename that). Only that uses pytest at all. And I really only use that with the newest version of Python to see if an idea is more efficient.

The project itself, and all actual tests (under tests/) fully support Python 3.9, and I plan on supporting it until mid 2027. My general rule is to support the minimum Python version for Python projects that are known to me, and Linux distributions that are still in support and have a recent version of pathspec in their repos.

[henryiii]

The problem largely was that a function accepting Histogram now accepted only Histogram[Double] unless someone went back and added something explicit. Ideally, you'd probably want something like C++'s auto or Rust's behavior with lifetimes, and get an implicit Histogram[T] where you don't get access to T unless you make it explicit. But it didn't work like that, it just becomes Histogram[Double] which is wrong for an input. Histogram[Any] is a better default for input parameters. The Generic is bound to Storage, so Any just means any Storage, just like this would be bound to Pattern.

[henryiii]

Ahh, I bet I know why the confusion with __getitem__ happened. The parameter is typed as Sequence | Iterable, so I bet the IDE type checker didn't narrow this to Sequence when it got assigned (mypy does, or at least it just believes the self.sequence: Sequence assignment), so that's why it thought there was no __getitem__.

[cpburnz]

What IDE are you using? Does it properly detect it as a sequence if self.patterns: Sequence[Pattern] = patterns is changed to self.patterns = cast(Sequence[Pattern], patterns)?