From 670dd891af948a169048eb9872fe95c581dc02c9 Mon Sep 17 00:00:00 2001 From: Gguidini Date: Fri, 2 Aug 2024 16:15:53 +1000 Subject: [PATCH] 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. --- .../new_notify/contexts/__init__.py | 56 +++++++++---------- .../new_notify/contexts/comment.py | 20 ++----- .../contexts/tests/test_contexts.py | 25 +++++++-- 3 files changed, 50 insertions(+), 51 deletions(-) diff --git a/services/bundle_analysis/new_notify/contexts/__init__.py b/services/bundle_analysis/new_notify/contexts/__init__.py index 6ec2be374..578aaff82 100644 --- a/services/bundle_analysis/new_notify/contexts/__init__.py +++ b/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": diff --git a/services/bundle_analysis/new_notify/contexts/comment.py b/services/bundle_analysis/new_notify/contexts/comment.py index ce28600c8..94efa4932 100644 --- a/services/bundle_analysis/new_notify/contexts/comment.py +++ b/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): diff --git a/services/bundle_analysis/new_notify/contexts/tests/test_contexts.py b/services/bundle_analysis/new_notify/contexts/tests/test_contexts.py index 0c78e9f62..b829125a8 100644 --- a/services/bundle_analysis/new_notify/contexts/tests/test_contexts.py +++ b/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