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       249766   249825   +59     
- Misses      62593    62599    +6     
- Partials     6361     6363    +2     

[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.