add GroupedMetadata as a base class for Interval

[adriangb]

The main idea here is to generalize the pattern in Interval so that Pydantic and similar can define their Field as inheriting from GroupedMetadata, thus anything that can parse annotated-types will know how to unpack it (if it is not already unpacked by the PEP-646) even if it is a custom subclass in a library like Pydantic.

Without this, some generic annotated-types parser would not know how to handle Pydantic's Field type without specific knowledge of Pydantic.

[codecov-commenter]

Codecov Report

Merging #12 (315d175) into main (12855f7) will increase coverage by 0.11%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main      #12      +/-   ##
==========================================
+ Coverage   95.77%   95.89%   +0.11%     
==========================================
  Files           2        2              
  Lines         142      146       +4     
  Branches       35       36       +1     
==========================================
+ Hits          136      140       +4     
  Misses          6        6              

Impacted Files	Coverage Δ
annotated_types/__init__.py	93.25% <100.00%> (+0.31%)	⬆️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

[Zac-HD]

Oh, very nice! I think that all this needs is documentation in the README as well as the lovely docstring. A few points to clarify:

• I'd be inclined to make iter an abstractmethod, so that downstream implementors actually do implement it.
• We should also specify that iter should (to the extent possible) fully describe the constraints this metadata poses in terms of standard annotated-types constraints, and may include other annotations.
• And then also specify that consumers of the annotated-types standard should check for GroupedMetadata instead of Interval etc.

After that, let's merge and release a new version!

[adriangb]

• I'd be inclined to make __iter__ an abstractmethod, so that downstream implementors actually do implement it.

Sounds good to me. Can just add an @abstractmethod without inheriting from abc.ABC and have type checkers enforce it, or do we need to add the metaclass? I forget.

• We should also specify that __iter__ should (to the extent possible) fully describe the constraints this metadata poses in terms of standard annotated-types constraints, and may include other annotations.
• And then also specify that consumers of the annotated-types standard should check for GroupedMetadata instead of Interval etc.

I think what you are saying is that GroupedMetadata should not have any meaning on its own, all of its information should be contained in the BaseMetadata's that get unpacked by __iter__(), right?

What do you think of having GroupedMetadata not inherit from BaseMetadata? I think it makes it clearer that GroupedMetadata in and of itself has no meaning.

[adriangb]

@Zac-HD I'm terribly sorry I hit edit instead of quote on your comment, so I edited yours instead of posting my own. I restored it now

[Zac-HD]

Sounds good to me. Can just add an @abstractmethod without inheriting from abc.ABC and have type checkers enforce it, or do we need to add the metaclass? I forget.

Have to add the metaclass, which is most easily done by inheriting from abc.ABC.

I think what you are saying is that GroupedMetadata should not have any meaning on its own, all of its information should be contained in the BaseMetadata's that get unpacked by __iter__(), right?

Yep.

What do you think of having GroupedMetadata not inherit from BaseMetadata? I think it makes it clearer that GroupedMetadata in and of itself has no meaning.

That sounds good to me!

That said, it seems useful to have a single checkable class. Maybe BaseMetadata should inherit from GroupedMetadata, and define __iter__ as yield self?

[adriangb]

Maybe BaseMetadata should inherit from GroupedMetadata, and define __iter__ as yield self?

That sounds nice in theory, but I'm weary of creating class hierarchies if I don't really need them. It's really no more LOC or complexity for consumers to have two checks nested or at the same level (you need 2 anyway), I'll push that version in a few min

[adriangb]

Pushed. Like I said the difference is minor, there are always going to be 2 isinstance checks, it's just about what level they are at:

https://github.com/annotated-types/annotated-types/pull/12/files#diff-41d180f6f7233d175c2dfe36c45c4ea80eed4754fa7ed4eed46ccde1b2a778c7

[Zac-HD]

Implementation LGTM, pending documentation.

[adriangb]

I started writing the docs and it felt a bit awkward because we don't really have any documentation discussing BaseMetadata or how one is supposed to "consume" any of the metadata in the first place. I think this warrants it's own section of the docs and the current section should be moved into a subheading Constraints or something like that. Does this sound reasonable, and would a followup PR be appropriate for this?

[Zac-HD]

Sounds reasonable; could be a follow-up or part of this PR; I'm happy with either. If follow-up, I think this is ready to merge (though not release without docs).

[samuelcolvin]

LGTM, my only concern is about use of abc, python/cpython#92810 - I basically live in fear of abs these days.

But I guess in this situation it:

1. Should be okay as the mro shouldn't get too deep
2. shouldn't be on a critical path
3. Is a perfect use for abc which would be much less clear without

[adriangb]

Sorry I merged right before your comment.

Agreed on avoiding ABCs, but I think we couldn't do much here. Protocol would be a no-go because Protocol + isinstance isn't great either.

We could always remove it and just manually raise a NotImplementedError or something.

[samuelcolvin]

We could always remove it and just manually raise a NotImplementedError or something.

That was going to be my suggestion, then the page updated as the PR was merged :-).

I think this is fine for now, surely abcs will have to be fixed in cpython soon + my above comments.

[adriangb]

If you feel strongly, we can still change it!

[Zac-HD]

(looks like re snuck back in somehow)

Happy to do whatever on abc, so long as the error is raised when subclassing, not when you try to iterate.

[samuelcolvin]

Happy to do whatever on abc, so long as the error is raised when subclassing, not when you try to iterate.

If we can do that, that's great with me. But is there a reliable (and fairly simple!) way to raise an error when defining subclass if a method hasn't been implemented?

If not, let's leave it as-is.

[adriangb]

You can implement __init_subclass__ to hook into subclassing, but this won't do anything for linters/static analysis.

[Zac-HD]

I'm not too worried about linters if we can get a fully-reliable runtime check, and it looks like __init_subclass__ would do that with less weird-abc-related performance concerns 👍

[adriangb]

I opened #16, we can continue that discussion there

[Zac-HD]

I like it enough to merge already 😆