ref(grouping): Add types to enhancement methods #87370
Conversation
github-actions
bot
added
the
Scope: Backend
Codecov ReportAttention: Patch coverage is ✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## master #87370 +/- ##
==========================================
- Coverage 87.74% 87.74% -0.01%
==========================================
Files 9896 9896
Lines 561312 561334 +22
Branches 22146 22146
==========================================
+ Hits 492510 492527 +17
- Misses 68398 68403 +5
Partials 404 404 |
| class Enhancements: | ||
| # NOTE: You must add a version to ``VERSIONS`` any time attributes are added | ||
| # to this class, s.t. no enhancements lacking these attributes are loaded | ||
| # from cache. | ||
| # See ``GroupingConfigLoader._get_enhancements`` in src/sentry/grouping/api.py. | ||
|
|
||
| def __init__( | ||
| self, rules, rust_enhancements: RustEnhancements, version=None, bases=None, id=None | ||
| self, | ||
| rules: list[EnhancementRule], |
There was a problem hiding this comment.
Please use abstract types when within the signature (e.g. Sequence) and more specific ones when returning (e.g. list). Unless it gets on the way for whatever reason.
There was a problem hiding this comment.
This is definitely not the hill I want to die on, and I can switch it if you feel strongly. But before I go through and make a bunch of changes:
In a purely theoretical sense I guess I understand the principle, but in practice, for methods used purely internally, it always feels like overkill to me. Is there a strong argument in favor that I'm missing?
There was a problem hiding this comment.
I won't push hard on this. It is mostly as documented in the best practice document. I can understand the argument of internal usage.
| @@ -287,7 +299,8 @@ def as_dict(self, with_rules=False): | |||
| rv["rules"] = [x.as_dict() for x in self.rules] | |||
| return rv | |||
|
|
|||
| def _to_config_structure(self): | |||
| def _to_config_structure(self) -> list[Any]: | |||
| # TODO: Can we switch this to a tuple so we can type it more exactly? | |||
There was a problem hiding this comment.
Maybe a named tuple?
from collections import namedtuple
ConfigStructure = namedtuple('ConfigStructure', ['version', 'bases', 'rules'])
return ConfigStructure(
version=self.version,
bases=self.bases,
rules=[x._to_config_structure(self.version) for x in self.rules],
)There was a problem hiding this comment.
Yup, that'd work. I'd rather leave it for a future PR, though, as my goal with this one is purely to add types, not change behavior.
lobsterkatie
force-pushed
the
kmclb-enhancement-types-prework
branch
from
2760276 to
4eb7eb8
Compare
lobsterkatie
force-pushed
the
kmclb-add-types-to-enhancements-code
branch
from
f742d58 to
570fff0
Compare
lobsterkatie
force-pushed
the
kmclb-enhancement-types-prework
branch
from
4eb7eb8 to
0c66767
Compare
lobsterkatie
force-pushed
the
kmclb-add-types-to-enhancements-code
branch
from
570fff0 to
bebfc24
Compare
lobsterkatie
force-pushed
the
kmclb-enhancement-types-prework
branch
from
0c66767 to
94395bd
Compare
lobsterkatie
force-pushed
the
kmclb-add-types-to-enhancements-code
branch
from
bebfc24 to
0478892
Compare
As a first step towards adding types to the enhancements code, this makes a handful of code changes which will be necessary once types are added to all of the enhancements methods, which will happen in #87370.
lobsterkatie
force-pushed
the
kmclb-add-types-to-enhancements-code
branch
from
0478892 to
a637739
Compare
This adds type hints for all of the enhancements methods, to increase the safety of upcoming refactors.