Make PyCollector an implementation detail - don't use in hook type annotation

[bluetech]

The pytest_pycollector_makeitem argument collector is currently annotated with type PyCollector. As part of #7469, that would have required us to expose it in the public API. But really it's an implementation detail, not something we want to expose. So replace the annotation with the concrete python collector types that are passed.

Strictly speaking, pytest_pycollect_makeitem is called from PyCollector.collect(), so the new type annotation is incorrect if another type subclasses PyCollector. But the set of python collectors is closed (mapping to language constructs), and the type is private, so there shouldn't be any other deriving classes, and we can consider it effectively sealed (unfortunately Python does not provide a way to express this - yet?).

[nicoddemus]

But the set of python collectors is closed (mapping to language constructs), and the type is private, so there shouldn't be any other deriving classes, and we can consider it effectively sealed

I thought it was intended to be able to subclass some of them (say, Module) to allow for some customizations, but I'm not 100% sure.

@RonnyPfannschmidt do you have something to add to that?

[RonnyPfannschmidt]

Initially those classes where exposed and subclassable/over rideable,

[bluetech]

Subclassing is not a problem, since a subclass of e.g. Module is still a Module.

A break can occur if a collector which subclasses PyCollector but doesn't subclass Module/Class/Instance. I found one case of this, in pytest-describe: https://github.com/pytest-dev/pytest-describe/blob/2b76f43f2fa6a0134c9590d96dd69c08203f4502/pytest_describe/plugin.py I'll try understand what it does more.

[nicoddemus]

Thanks, just commented on that for clarification, as I misunderstood your description as Module, Package, etc, could not be subclassed. 👍

[bluetech]

I rebased to remove Instance from the union, and sent a PR to pytest-describe to avoid PyCollector.

[bluetech]

The pytest-describe is merged. pytest-describe is pretty interesting, it essentially adds an extra Python namespacing layer -- a "describe block" -- by sort of embedding a module inside a function. But it now inherits from Module since it really creates a module behind the scenes.

So this should be good to go if we want to go with hiding PyCollector.

[The-Compiler]

What's the state here? Should this go into 7.0?

[bluetech]

What's the state here?

Waiting for review.

Should this go into 7.0?

Not really, can go after.

[nicoddemus]

Suggested change

	"""Return a custom item/collector for a Python object in a module, or None.
	"""Return a custom item/collector for a Python object in a module or class, or None.