ref(grouping): Add GroupingComponent subclasses

[lobsterkatie]

This adds a full taxonomy of GroupingCompontent subclasses, partially just as a form of documentation (we create this deeply nested variants structure when grouping, and it's helpful to understand what kind of data is in each part), and partially so that it's easier to write helpers dealing with different kinds of components and not have to constantly be checking for the presence of this or that attribute.

In order to support the subclassing, as few other changes have been made:

• Rename GroupindComponent to BaseGroupingComponent.
• Add class-level type declarations to BaseGroupingComponent.
• Allow id to be a class attribute (no longer require it to be passed to __init__).
• Use the class name (rather than hardcoded GroupingComponent) in __repr__.
• Add int to the values list type. (This is more of a bug fix, though it's also a necessary for the typing of NSErrorGroupingComponent. In fact, values has always been able to contain ints, but the incoming data isn't super strictly typed, so typechecking has passed even with this missing.)

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

✅ All tests successful. No failed tests found.

Additional details and impacted files

@@           Coverage Diff           @@
##           master   #80503   +/-   ##
=======================================
  Coverage   78.36%   78.36%           
=======================================
  Files        7206     7206           
  Lines      318720   318787   +67     
  Branches    43905    43905           
=======================================
+ Hits       249771   249825   +54     
- Misses      62589    62599   +10     
- Partials     6360     6363    +3     

[JoshFerge]

I'm overall wondering if the subclassing change is worth the additional complexity.

• introducing a "family tree" of subclasses feels somewhat brittle and difficult to understand https://en.wikipedia.org/wiki/Composition_over_inheritance
• as written, the typing does not actually enforce anything, which may lead developers down the line to be confused / rely upon it for checking values, when in reality it is not doing so. (the init method and update method do not actually validate the types of what's passed in)
• existing code makes sense with the string implementation, but when switching to class, it feels more confusing. e.g. _security_v1 now has to take in a type of a component, which just feels very strange.

I get the want to have typing to make things feel more correct, but as written, i feel as though this will end up more complex than it needs to be.

here is one idea that could give us some self-documenting code and actually enforce values types, albeit during runtime, not during type checking. 388b1b2

[JoshFerge]

Adding a bit of color to the above:

Basically what it comes down to is basically I'm advocating for "replace type code with class" https://refactoring.guru/replace-type-code-with-class. while the current implementation is "Replace typecode with subclass https://refactoring.guru/replace-type-code-with-subclasses".

My arguments for "replace type code with class" are

• Sidestep all brittleness / confusion / complication with python's current type system and our pre-existing implementation
• the current implementation does not actually enforce the types its setting
• avoiding a somewhat complex inheritance hierarchy
• use enum to constrain the values of id
• can do runtime check using map of Enums / "ALLOW_PRIMITIVES" to enforce correctness, 388b1b2 as well as documenting it in code
• Given current implementation, we aren't taking advantage of polymorphism, as in feat(grouping): Store grouping-method-specific metadata in GroupHashMetadata #80534 we're still having to do isinstance checks, so we aren't gaining a whole lot in my mind

Arguments for "replace type-code with subclass"

• Could eventually in theory validate values types at type-checking time instead of run-time
• could eventually take advantage of more polymorphic patterns moving things to instance methods, and removing various isinstance calls
• in helper functions feat(grouping): Store grouping-method-specific metadata in GroupHashMetadata #80534 it makes some parts easier as we know what's in the values

[armenzg]

I see what you're aiming at but as Josh pointed out this is not a good design pattern to follow.

[lobsterkatie]

So, prompted by Josh, I made a few changes.

TL;DR, using a generic worked this time, so now all of the subclasses just inherit from BaseGroupingComponent, with different types provided for its type parameter. This means types are actually being enforced at the base class level, both in __init__ and in .update(). (In fact, implementing this led me to discover a few existing type issues, which are fixed here and here.)

I also did the following:

• Switched the security strategies so they're just like the any other strategy, in that they just create a component directly, rather than relying on the helper to which I was having to pass the class type. That's here.
• Pulled the changes from this PR needed by but not directly related to the subclassing out into a separate PR, since they're things which can happen regardless. That's here.
• Just to reduce the noise, pulled the renaming of GroupingComponent to BaseGroupingComponent out as well. Given that it really only makes sense if the subclassing is approved, I won't merge it until we're agreed on that. That's here.
• Because that all took some rearranging of branches, I opened a new PR with just the subclassing here.

Hopefully that addresses the concerns? Please let me know if not!

(Note: Right now all the PRs are stacked, because they're all necessary for the subclass changes, but most of them are independent, so once we're all set I'll rebase them on to master separately so their CI can run in parallel and I can get them merged.)

Oh, also: The one remaining typing question I had: I don't love that I have to type contributing_variant and contributing_component here as Any. I wanted to be able to associate the correct type for each with the hash_basis, but including the types in GROUPING_METHODS_BY_DESCRIPTION didn't work, because you can't pass anything but a literal type to cast. Definitely open to suggestions if there's a nice way to make that work. (UPDATE, as I'm typing this: Overloads? Maybe overloads. Lemme try that.)

[JoshFerge]

can this be made abstract?

[JoshFerge]

why would we ever pass in id if there are specific subclasses for each grouping component?

[JoshFerge]

@lobsterkatie i don't think the changes you're mentioning in the above comment are pushed up?

[lobsterkatie]

@lobsterkatie i don't think the changes you're mentioning in the above comment are pushed up?

Sorry, maybe this wasn't clear:

Because that all took some rearranging of branches, I opened a new PR with just the subclassing here.

It just seemed easier, given that I breaking things up, to make a new PR stack. Let me close this one to make it clearer. (I'll also answer your other questions there.)