address review comments

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.

services/bundle_analysis/new_notify/contexts/__init__.py

@@ -1,4 +1,5 @@
 from functools import cached_property
+from typing import Generic, TypeVar
 
 from shared.bundle_analysis import (
     BundleAnalysisReport,
@@ -17,6 +18,27 @@
 )
 from services.storage import get_storage_client
 
+T = TypeVar("T")
+
+
+class ContextNotLoadedError(Exception):
+    pass
+
+
+class NotificationContextField(Generic[T]):
+    def __set_name__(self, owner, name) -> None:
+        self._name = name
+
+    def __get__(self, instance: "NotificationContextField", owner) -> T:
+        if self._name not in instance.__dict__:
+            raise ContextNotLoadedError(
+                "The property you are trying to access is not loaded. Make sure to build the context before using it."
+            )
+        return instance.__dict__[self._name]
+
+    def __set__(self, instance: "NotificationContextField", value: T) -> None:
+        instance.__dict__[self._name] = value
+
 
 class BaseBundleAnalysisNotificationContext:
     """Base NotificationContext for bundle analysis notifications.
@@ -48,21 +70,10 @@ def repository_service(self) -> TorngitBaseAdapter:
             installation_name_to_use=self.gh_app_installation_name,
         )
 
-    @property
-    def commit_report(self) -> CommitReport:
-        return self._commit_report
-
-    @commit_report.setter
-    def commit_report(self, report: CommitReport) -> None:
-        self._commit_report = report
-
-    @property
-    def bundle_analysis_report(self) -> BundleAnalysisReport:
-        return self._bundle_analysis_report
-
-    @bundle_analysis_report.setter
-    def bundle_analysis_report(self, report: BundleAnalysisReport) -> None:
-        self._bundle_analysis_report = report
+    commit_report: CommitReport = NotificationContextField[CommitReport]()
+    bundle_analysis_report: BundleAnalysisReport = NotificationContextField[
+        BundleAnalysisReport
+    ]()
 
 
 class NotificationContextBuildError(Exception):
@@ -79,21 +90,6 @@ class WrongContextBuilderError(Exception):
 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)
-    # The annoying bit is creating a specialized class from the base one without having to nitpick the details
-    # @classmethod
-    # def clone_builder(
-    #     cls, builder: "NotificationContextBuilder"
-    # ) -> "NotificationContextBuilder":
-    #     context_copy = deepcopy(builder._notification_context)
-    #     new_builder = cls()
-    #     new_builder._notification_context = context_copy
-    #     new_builder.commit_report_loaded = builder.commit_report_loaded
-    #     new_builder.bundle_analysis_report_loaded = (
-    #         builder.bundle_analysis_report_loaded
-    #     )
-    #     return new_builder
-
     def initialize(
         self, commit: Commit, current_yaml: UserYaml, gh_app_installation_name: str
     ) -> "NotificationContextBuilder":

services/bundle_analysis/new_notify/contexts/comment.py

@@ -19,6 +19,7 @@
     BaseBundleAnalysisNotificationContext,
     NotificationContextBuilder,
     NotificationContextBuildError,
+    NotificationContextField,
 )
 from services.bundle_analysis.new_notify.types import NotificationType
 from services.repository import (
@@ -34,21 +35,10 @@ class BundleAnalysisCommentNotificationContext(BaseBundleAnalysisNotificationCon
 
     notification_type = NotificationType.PR_COMMENT
 
-    @property
-    def pull(self) -> EnrichedPull:
-        return self._pull
-
-    @pull.setter
-    def pull(self, pull: EnrichedPull):
-        self._pull = pull
-
-    @property
-    def bundle_analysis_comparison(self) -> BundleAnalysisComparison:
-        return self._bundle_analysis_comparison
-
-    @bundle_analysis_comparison.setter
-    def bundle_analysis_comparison(self, comparison: BundleAnalysisComparison):
-        self._bundle_analysis_comparison = comparison
+    pull: EnrichedPull = NotificationContextField[EnrichedPull]()
+    bundle_analysis_comparison: BundleAnalysisComparison = NotificationContextField[
+        BundleAnalysisComparison
+    ]()
 
 
 class BundleAnalysisCommentContextBuilder(NotificationContextBuilder):

services/bundle_analysis/new_notify/contexts/tests/test_contexts.py

@@ -12,6 +12,7 @@
     save_mock_bundle_analysis_report,
 )
 from services.bundle_analysis.new_notify.contexts import (
+    ContextNotLoadedError,
     NotificationContextBuilder,
     NotificationContextBuildError,
 )
@@ -21,6 +22,18 @@
 
 
 class TestBaseBundleAnalysisNotificationContextBuild:
+    def test_access_not_loaded_field_raises(self, dbsession):
+        head_commit, _ = get_commit_pair(dbsession)
+        builder = NotificationContextBuilder().initialize(
+            head_commit, UserYaml.from_dict({}), GITHUB_APP_INSTALLATION_DEFAULT_NAME
+        )
+        with pytest.raises(ContextNotLoadedError) as exp:
+            builder._notification_context.commit_report
+        assert (
+            str(exp.value)
+            == "The property you are trying to access is not loaded. Make sure to build the context before using it."
+        )
+
     def test_load_commit_report_no_report(self, dbsession):
         head_commit, _ = get_commit_pair(dbsession)
         builder = NotificationContextBuilder().initialize(
@@ -192,11 +205,11 @@ def test_evaluate_changes_fail(self, config, total_size_delta, dbsession, mocker
             name="fake_pull",
             database_pull=MagicMock(bundle_analysis_commentid=None, id=12),
         )
-        builder._notification_context._pull = mock_pull
+        builder._notification_context.pull = mock_pull
         mock_comparison = MagicMock(
             name="fake_bundle_analysis_comparison", total_size_delta=total_size_delta
         )
-        builder._notification_context._bundle_analysis_comparison = mock_comparison
+        builder._notification_context.bundle_analysis_comparison = mock_comparison
         with pytest.raises(NotificationContextBuildError) as exp:
             builder.evaluate_has_enough_changes()
         assert exp.value.failed_step == "evaluate_has_enough_changes"
@@ -247,11 +260,11 @@ def test_evaluate_changes_success(self, config, total_size_delta, dbsession):
             name="fake_pull",
             database_pull=MagicMock(bundle_analysis_commentid=None, id=12),
         )
-        builder._notification_context._pull = mock_pull
+        builder._notification_context.pull = mock_pull
         mock_comparison = MagicMock(
             name="fake_bundle_analysis_comparison", total_size_delta=total_size_delta
         )
-        builder._notification_context._bundle_analysis_comparison = mock_comparison
+        builder._notification_context.bundle_analysis_comparison = mock_comparison
         result = builder.evaluate_has_enough_changes()
         assert result == builder
 
@@ -272,11 +285,11 @@ def test_evaluate_changes_comment_exists(self, dbsession):
             name="fake_pull",
             database_pull=MagicMock(bundle_analysis_commentid=12345, id=12),
         )
-        builder._notification_context._pull = mock_pull
+        builder._notification_context.pull = mock_pull
         mock_comparison = MagicMock(
             name="fake_bundle_analysis_comparison", total_size_delta=100
         )
-        builder._notification_context._bundle_analysis_comparison = mock_comparison
+        builder._notification_context.bundle_analysis_comparison = mock_comparison
         result = builder.evaluate_has_enough_changes()
         assert result == builder