refactor: contexts and builders #579
Conversation
Codecov ReportAttention: Patch coverage is
Changes have been made to critical files, which contain lines commonly executed in production. Learn more ✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## main #579 +/- ##
==========================================
+ Coverage 97.57% 97.62% +0.05%
==========================================
Files 455 460 +5
Lines 36440 37074 +634
==========================================
+ Hits 35556 36195 +639
+ Misses 884 879 -5
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 6 files with indirect coverage changes This change has been scanned for critical changes. Learn more |
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
@@ Coverage Diff @@
## main #579 +/- ##
==========================================
+ Coverage 97.53% 97.55% +0.02%
==========================================
Files 420 425 +5
Lines 35234 35593 +359
==========================================
+ Hits 34364 34724 +360
+ Misses 870 869 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Codecov ReportAttention: Patch coverage is ✅ All tests successful. No failed tests found.
@@ Coverage Diff @@
## main #579 +/- ##
==========================================
+ Coverage 97.50% 97.55% +0.05%
==========================================
Files 414 425 +11
Lines 35002 35593 +591
==========================================
+ Hits 34127 34724 +597
+ Misses 875 869 -6
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found. @@ Coverage Diff @@
## main #579 +/- ##
==========================================
+ Coverage 97.53% 97.55% +0.02%
==========================================
Files 420 425 +5
Lines 35234 35593 +359
==========================================
+ Hits 34364 34724 +360
+ Misses 870 869 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
giovanni-guidini
force-pushed
the
gio/refactor/ba-notifier/helpers
branch
2 times, most recently
from
d335c05 to
a22bdab
Compare
giovanni-guidini
force-pushed
the
gio/refactor/ba-notifier/contexts
branch
from
e7eee09 to
1a378e4
Compare
introduces the NotificationContext and ContextBuilder classes. the only specialization currently is for the PR comment. the context builders generate the context details necessary to create and send the message later on. Each step is called in order, and if one step fails we bail with an exception. This means the notification can't be sent.
giovanni-guidini
force-pushed
the
gio/refactor/ba-notifier/contexts
branch
from
1a378e4 to
1c41841
Compare
| class NotificationContextBuilder: | ||
| """Creates the BaseBundleAnalysisNotificationContext one step at a time, in the correct order.""" | ||
|
|
||
| # TODO: Find a way to re-use the base-context (by cloning it instead of recreating it for every notification) |
There was a problem hiding this comment.
Could we try a singleton pattern approach perhaps? Make it once, save it, and if it exists already, use it?
There was a problem hiding this comment.
Singleton I don't think was the answer.
I did something tho, in which you can initialize the builder from an existing context by looking at what fields the existing context has and porting them over to the new one.
Because notification is a read-only process it's OK to have the same reference in multiple places.
There was a problem hiding this comment.
Huh, does this have a pattern?? Not really familiar w/ this
| log = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| class BundleAnalysisCommentNotificationContext(BaseBundleAnalysisNotificationContext): |
There was a problem hiding this comment.
Optional: since we call the notification type PR_COMMENT, would it make sense to call this BundleAnalysisPRCommentNotificationContext comment instead?
| ) | ||
| return self | ||
| comparison = self._notification_context.bundle_analysis_comparison | ||
| should_continue = { |
There was a problem hiding this comment.
I found this a bit hard to read at first - the False: True broke my brain 😂, but after going through it see why you did it this way. Perhaps a case statement could help with readability, I trust your judgement 👍
There was a problem hiding this comment.
It's funny you say that because I was originally going to go with a match statement. But you can't directly assign to a variable using that.
Looking online some stack overflow question suggested this pattern and I was like "ok cool"
There was a problem hiding this comment.
Yeaah I suppose you'd have to write the return statement in each case instead of assigning a variable. We can leave it as is
Create a descriptor for notification context fields so that we fail with a custom exception if that field was not loaded but we are trying to access it. With the added benefit of reducing the amount of code a little bit :E I added typehints to the fields (e.g. `pull: EnrichedPull = NotificationContextField[EnrichedPull]()`) because my VSCode langauge server was not being helpful with the generics... but leaving the generics in any case.
giovanni-guidini
force-pushed
the
gio/refactor/ba-notifier/contexts
branch
from
b72165c to
670dd89
Compare
This lets us only load the necessary info once and re-use the calculations for subsequent contexts that need to be built.
| pass | ||
|
|
||
|
|
||
| class NotificationContextField(Generic[T]): |
There was a problem hiding this comment.
ahh python descriptors, i remember you
this appears to be a very straightforward use of the pattern, but could you add a doc comment saying saying, like
NotificationContextFieldis a descriptor akin to a Django model field. If you create one as a class member namedfoo, it will define the behavior to get and set an instance member namedfoo.
just to help out folks who can't recognize the pattern on sight (i had to google around to remember)
| notification_context = builder.build_context().get_result() | ||
| """ | ||
|
|
||
| notification_type: NotificationType |
There was a problem hiding this comment.
yes... I think to notify when the context fails to build. Might be in a later PR
| def build_context(self) -> "BundleAnalysisCommentContextBuilder": | ||
| super().build_context() | ||
| async_to_sync(self.load_enriched_pull)() | ||
| self.load_bundle_comparison() | ||
| self.evaluate_has_enough_changes() | ||
| return self |
There was a problem hiding this comment.
all of these functions return self, but you're not really using that. also, none of them take arguments. every creation of a NotificationContext is going to go the same way. so you don't really need the builder pattern exactly
you could probably just do all this in a constructor?
class BaseBundleAnalysisNotificationContext():
def __init__(self, commit: Commit, current_yaml: UserYaml, gh_app_installation_name: str) -> None:
self.commit = commit
self.current_yaml = current_yaml
self.gh_app_installation_name = gh_app_installation_name
self.load_commit_report()
self.load_bundle_analysis_report()
def load_commit_report(self):
pass
def load_bundle_analysis_report(self):
pass
class BundleAnalysisCommentNotificationContext(BaseBundleAnalysisNotificationContext):
def __init__(self, commit: Commit, current_yaml: UserYaml, gh_app_installation_name: str) -> None:
super(BaseBundleAnalysisNotificationContext, self).__init__()
async_to_sync(self.load_enriched_pull)()
self.load_bundle_comparison()
self.evaluate_has_enough_changes()
async def load_enriched_pull(self):
pass
def load_bundle_comparison(self):
pass
def evaluate_has_enough_changes(self):
pass
though i guess that makes it hard to test the individual member functions. maybe keep the builder then. idk
There was a problem hiding this comment.
so you don't really need the builder pattern exactly
You're probably right... I do want to avoid the load function being declared in the Context though... I would prefer to have the context be more similar to a dataclass.
I think using composition for the load steps (they are declared in different classes that are composed into the context) and the context itself declared the build steps might reduce the amount of code we have and overall be more flexible and easier to reuse steps
And we get rid of the descriptor too
There was a problem hiding this comment.
Maybe I stop messing with it and we see how this feels. It feels a clearer interface in any case so it's an improvement
* improve docstrings * change bundle analysis PR comment context name
introduces the NotificationContext and ContextBuilder classes.
the only specialization currently is for the PR comment.
the context builders generate the context details necessary to create and
send the message later on. Each step is called in order, and if one step fails
we bail with an exception. This means the notification can't be sent.