From cd01776f4497215a4938c7322f18d2a4519f8e70 Mon Sep 17 00:00:00 2001 From: mariagrimaldi Date: Wed, 28 Jul 2021 18:34:52 -0400 Subject: [PATCH 01/10] feat: add inline code-annotations for Open edX Events --- edx_lint/pylint/annotations_check.py | 182 ++++++++++++++++++++++++++- 1 file changed, 181 insertions(+), 1 deletion(-) diff --git a/edx_lint/pylint/annotations_check.py b/edx_lint/pylint/annotations_check.py index b23d66d..a260dac 100644 --- a/edx_lint/pylint/annotations_check.py +++ b/edx_lint/pylint/annotations_check.py @@ -22,6 +22,7 @@ def register_checkers(linter): linter.register_checker(CodeAnnotationChecker(linter)) linter.register_checker(FeatureToggleAnnotationChecker(linter)) linter.register_checker(SettingAnnotationChecker(linter)) + linter.register_checker(EventsAnnotationChecker(linter)) def check_all_messages(msgs): @@ -296,7 +297,7 @@ class CodeAnnotationChecker(AnnotationBaseChecker): When creating a new annotation configuration, its filename should be added to CodeAnnotationChecker.CONFIG_FILENAMES (see AnnotationBaseChecker docs). """ - CONFIG_FILENAMES = ["feature_toggle_annotations.yaml", "setting_annotations.yaml"] + CONFIG_FILENAMES = ["feature_toggle_annotations.yaml", "setting_annotations.yaml", "events_annotations.yaml"] name = "code-annotations" msgs = { ("E%d%d" % (BASE_ID, index + 50)): ( @@ -570,3 +571,182 @@ def check_annotation_group(self, search, annotations, node): node=node, line=line_number, ) + + +class EventsAnnotationChecker(AnnotationBaseChecker): + """ + Perform checks on events annotations. + """ + + CONFIG_FILENAMES = ["events_annotations.yaml"] + + __implements__ = (IAstroidChecker,) + + name = "events-annotations" + + NO_TYPE_MESSAGE_ID = "event-no-type" + NO_NAME_MESSAGE_ID = "event-no-name" + NO_DATA_MESSAGE_ID = "event-no-data" + NO_STATUS_MESSAGE_ID = "event-empty-description" + NO_DESCRIPTION_MESSAGE_ID = "event-empty-description" + MISSING_ANNOTATION = "event-missing-annotation" + + msgs = { + ("E%d80" % BASE_ID): ( + "event annotation has no type", + NO_TYPE_MESSAGE_ID, + "Events annotations type must be present and be the first annotation", + ), + ("E%d81" % BASE_ID): ( + "event annotation (%s) has no name", + NO_NAME_MESSAGE_ID, + "Events annotations name must be present", + ), + ("E%d82" % BASE_ID): ( + "event annotation (%s) has no data argument", + NO_DATA_MESSAGE_ID, + "Events annotations must include data argument", + ), + ("E%d82" % BASE_ID): ( + "event annotation (%s) has no status", + NO_STATUS_MESSAGE_ID, + "Events annotations must include status of event", + ), + ("E%d83" % BASE_ID): ( + "event annotation (%s) does not have a description", + NO_DESCRIPTION_MESSAGE_ID, + "Events annotations must include a short description", + ), + ("E%d84" % BASE_ID): ( + "missing event annotation", + MISSING_ANNOTATION, + ( + "When an Open edX event object is created, a corresponding annotation must be present above in the" + " same module and with a matching name", + ) + ), + } + + EVENT_CLASS_NAMES = ["OpenEdxPublicSignal"] + + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + self.current_module_annotated_event_names = set() + self.current_module_annotation_group_line_numbers = [] + + @check_all_messages(msgs) + def visit_module(self, node): + """ + Run all checks on a single module. + """ + self.check_module(node) + + def leave_module(self, _node): + self.current_module_annotated_event_names.clear() + self.current_module_annotation_group_line_numbers.clear() + + def check_annotation_group(self, search, annotations, node): + """ + Perform checks on a single annotation group. + """ + if not annotations: + return + + event_type = "" + event_name = "" + event_data = "" + event_description = "" + line_number = None + for annotation in annotations: + if line_number is None: + line_number = annotation["line_number"] + self.current_module_annotation_group_line_numbers.append(line_number) + if annotation["annotation_token"] == ".. event_type:": + event_type = annotation["annotation_data"] + elif annotation["annotation_token"] == ".. event_name:": + event_name = annotation["annotation_data"] + self.current_module_annotated_event_names.add(event_name) + elif annotation["annotation_token"] == ".. event_data:": + event_data = annotation["annotation_data"] + elif annotation["annotation_token"] == ".. event_description:": + event_description = annotation["annotation_data"] + elif annotation["annotation_token"] == ".. event_status:": + event_status = annotation["annotation_data"] + + if not event_type: + self.add_message( + self.NO_TYPE_MESSAGE_ID, + node=node, + line=line_number, + ) + + if not event_name: + self.add_message( + self.NO_NAME_MESSAGE_ID, + args=(event_type,), + node=node, + line=line_number, + ) + + if not event_data: + self.add_message( + self.NO_DATA_MESSAGE_ID, + args=(event_type,), + node=node, + line=line_number, + ) + + if not event_description: + self.add_message( + self.NO_DESCRIPTION_MESSAGE_ID, + args=(event_type,), + node=node, + line=line_number, + ) + + if not event_status: + self.add_message( + self.NO_STATUS_MESSAGE_ID, + args=(event_type,), + node=node, + line=line_number, + ) + + + @utils.check_messages(MISSING_ANNOTATION) + def visit_call(self, node): + """ + Check for missing annotations. + """ + if self.is_annotation_missing(node): + self.add_message( + self.MISSING_ANNOTATION, + node=node, + ) + + def is_annotation_missing(self, node): + """ + Check whether the node corresponds to a toggle instance creation. if yes, check that it is annotated. + """ + if ( + not isinstance(node.func, Name) + or node.func.name not in self.EVENT_CLASS_NAMES + ): + return False + + if not self.current_module_annotation_group_line_numbers: + # There are no annotations left + return True + + annotation_line_number = self.current_module_annotation_group_line_numbers[0] + if annotation_line_number > node.tolineno: + # The next annotation is located after the current node + return True + self.current_module_annotation_group_line_numbers.pop(0) + + # Check literal event name arguments + if node.args and isinstance(node.args[0], Const) and isinstance(node.args[0].value, str): + event_name = node.args[0].value + if event_name not in self.current_module_annotated_event_names: + return True + return False From 3dc1a67df519beead03a8d649c56f87bc4e596ce Mon Sep 17 00:00:00 2001 From: mariagrimaldi Date: Fri, 30 Jul 2021 08:38:25 -0400 Subject: [PATCH 02/10] fix: make events annotation an optional plugin --- edx_lint/pylint/annotations_check.py | 180 ---------------- edx_lint/pylint/events_annotation/__init__.py | 11 + .../events_annotation_check.py | 194 ++++++++++++++++++ 3 files changed, 205 insertions(+), 180 deletions(-) create mode 100644 edx_lint/pylint/events_annotation/__init__.py create mode 100644 edx_lint/pylint/events_annotation/events_annotation_check.py diff --git a/edx_lint/pylint/annotations_check.py b/edx_lint/pylint/annotations_check.py index a260dac..9af40c9 100644 --- a/edx_lint/pylint/annotations_check.py +++ b/edx_lint/pylint/annotations_check.py @@ -22,7 +22,6 @@ def register_checkers(linter): linter.register_checker(CodeAnnotationChecker(linter)) linter.register_checker(FeatureToggleAnnotationChecker(linter)) linter.register_checker(SettingAnnotationChecker(linter)) - linter.register_checker(EventsAnnotationChecker(linter)) def check_all_messages(msgs): @@ -571,182 +570,3 @@ def check_annotation_group(self, search, annotations, node): node=node, line=line_number, ) - - -class EventsAnnotationChecker(AnnotationBaseChecker): - """ - Perform checks on events annotations. - """ - - CONFIG_FILENAMES = ["events_annotations.yaml"] - - __implements__ = (IAstroidChecker,) - - name = "events-annotations" - - NO_TYPE_MESSAGE_ID = "event-no-type" - NO_NAME_MESSAGE_ID = "event-no-name" - NO_DATA_MESSAGE_ID = "event-no-data" - NO_STATUS_MESSAGE_ID = "event-empty-description" - NO_DESCRIPTION_MESSAGE_ID = "event-empty-description" - MISSING_ANNOTATION = "event-missing-annotation" - - msgs = { - ("E%d80" % BASE_ID): ( - "event annotation has no type", - NO_TYPE_MESSAGE_ID, - "Events annotations type must be present and be the first annotation", - ), - ("E%d81" % BASE_ID): ( - "event annotation (%s) has no name", - NO_NAME_MESSAGE_ID, - "Events annotations name must be present", - ), - ("E%d82" % BASE_ID): ( - "event annotation (%s) has no data argument", - NO_DATA_MESSAGE_ID, - "Events annotations must include data argument", - ), - ("E%d82" % BASE_ID): ( - "event annotation (%s) has no status", - NO_STATUS_MESSAGE_ID, - "Events annotations must include status of event", - ), - ("E%d83" % BASE_ID): ( - "event annotation (%s) does not have a description", - NO_DESCRIPTION_MESSAGE_ID, - "Events annotations must include a short description", - ), - ("E%d84" % BASE_ID): ( - "missing event annotation", - MISSING_ANNOTATION, - ( - "When an Open edX event object is created, a corresponding annotation must be present above in the" - " same module and with a matching name", - ) - ), - } - - EVENT_CLASS_NAMES = ["OpenEdxPublicSignal"] - - def __init__(self, *args, **kwargs): - super().__init__(*args, **kwargs) - self.current_module_annotated_event_names = set() - self.current_module_annotation_group_line_numbers = [] - - @check_all_messages(msgs) - def visit_module(self, node): - """ - Run all checks on a single module. - """ - self.check_module(node) - - def leave_module(self, _node): - self.current_module_annotated_event_names.clear() - self.current_module_annotation_group_line_numbers.clear() - - def check_annotation_group(self, search, annotations, node): - """ - Perform checks on a single annotation group. - """ - if not annotations: - return - - event_type = "" - event_name = "" - event_data = "" - event_description = "" - line_number = None - for annotation in annotations: - if line_number is None: - line_number = annotation["line_number"] - self.current_module_annotation_group_line_numbers.append(line_number) - if annotation["annotation_token"] == ".. event_type:": - event_type = annotation["annotation_data"] - elif annotation["annotation_token"] == ".. event_name:": - event_name = annotation["annotation_data"] - self.current_module_annotated_event_names.add(event_name) - elif annotation["annotation_token"] == ".. event_data:": - event_data = annotation["annotation_data"] - elif annotation["annotation_token"] == ".. event_description:": - event_description = annotation["annotation_data"] - elif annotation["annotation_token"] == ".. event_status:": - event_status = annotation["annotation_data"] - - if not event_type: - self.add_message( - self.NO_TYPE_MESSAGE_ID, - node=node, - line=line_number, - ) - - if not event_name: - self.add_message( - self.NO_NAME_MESSAGE_ID, - args=(event_type,), - node=node, - line=line_number, - ) - - if not event_data: - self.add_message( - self.NO_DATA_MESSAGE_ID, - args=(event_type,), - node=node, - line=line_number, - ) - - if not event_description: - self.add_message( - self.NO_DESCRIPTION_MESSAGE_ID, - args=(event_type,), - node=node, - line=line_number, - ) - - if not event_status: - self.add_message( - self.NO_STATUS_MESSAGE_ID, - args=(event_type,), - node=node, - line=line_number, - ) - - - @utils.check_messages(MISSING_ANNOTATION) - def visit_call(self, node): - """ - Check for missing annotations. - """ - if self.is_annotation_missing(node): - self.add_message( - self.MISSING_ANNOTATION, - node=node, - ) - - def is_annotation_missing(self, node): - """ - Check whether the node corresponds to a toggle instance creation. if yes, check that it is annotated. - """ - if ( - not isinstance(node.func, Name) - or node.func.name not in self.EVENT_CLASS_NAMES - ): - return False - - if not self.current_module_annotation_group_line_numbers: - # There are no annotations left - return True - - annotation_line_number = self.current_module_annotation_group_line_numbers[0] - if annotation_line_number > node.tolineno: - # The next annotation is located after the current node - return True - self.current_module_annotation_group_line_numbers.pop(0) - - # Check literal event name arguments - if node.args and isinstance(node.args[0], Const) and isinstance(node.args[0].value, str): - event_name = node.args[0].value - if event_name not in self.current_module_annotated_event_names: - return True - return False diff --git a/edx_lint/pylint/events_annotation/__init__.py b/edx_lint/pylint/events_annotation/__init__.py new file mode 100644 index 0000000..f7e7a34 --- /dev/null +++ b/edx_lint/pylint/events_annotation/__init__.py @@ -0,0 +1,11 @@ +""" +edx_lint events_annotation module (optional plugin for events inline code-annotations +checks). + +Add this to your pylintrc:: + load-plugins=edx_lint.pylint.events_annotation +""" + +from .events_annotation_check import register_checkers + +register = register_checkers diff --git a/edx_lint/pylint/events_annotation/events_annotation_check.py b/edx_lint/pylint/events_annotation/events_annotation_check.py new file mode 100644 index 0000000..04048bf --- /dev/null +++ b/edx_lint/pylint/events_annotation/events_annotation_check.py @@ -0,0 +1,194 @@ +""" +Pylint plugin: checks that Open edX Events are properly annotated. +""" + +from pylint.interfaces import IAstroidChecker +from pylint.checkers import utils + +from edx_lint.pylint.common import BASE_ID +from edx_lint.pylint.annotations_check import AnnotationBaseChecker, check_all_messages +from astroid.node_classes import Const, Name + +def register_checkers(linter): + """ + Register checkers. + """ + linter.register_checker(EventsAnnotationChecker(linter)) + +class EventsAnnotationChecker(AnnotationBaseChecker): + """ + Perform checks on events annotations. + """ + + CONFIG_FILENAMES = ["events_annotations.yaml"] + + __implements__ = (IAstroidChecker,) + + name = "events-annotations" + + NO_TYPE_MESSAGE_ID = "event-no-type" + NO_NAME_MESSAGE_ID = "event-no-name" + NO_DATA_MESSAGE_ID = "event-no-data" + NO_STATUS_MESSAGE_ID = "event-no-status" + NO_DESCRIPTION_MESSAGE_ID = "event-empty-description" + MISSING_ANNOTATION = "event-missing-annotation" + + msgs = { + ("E%d80" % BASE_ID): ( + "event annotation has no type", + NO_TYPE_MESSAGE_ID, + "Events annotations type must be present and be the first annotation", + ), + ("E%d81" % BASE_ID): ( + "event annotation (%s) has no name", + NO_NAME_MESSAGE_ID, + "Events annotations name must be present", + ), + ("E%d82" % BASE_ID): ( + "event annotation (%s) has no data argument", + NO_DATA_MESSAGE_ID, + "Events annotations must include data argument", + ), + ("E%d83" % BASE_ID): ( + "event annotation (%s) has no status", + NO_STATUS_MESSAGE_ID, + "Events annotations must include the status of event", + ), + ("E%d84" % BASE_ID): ( + "event annotation (%s) does not have a description", + NO_DESCRIPTION_MESSAGE_ID, + "Events annotations must include a short description", + ), + ("E%d85" % BASE_ID): ( + "missing event annotation", + MISSING_ANNOTATION, + ( + "When an Open edX event object is created, a corresponding annotation must be present above in the" + " same module and with a matching name", + ) + ), + } + + EVENT_CLASS_NAMES = ["OpenEdxPublicSignal"] + + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + self.current_module_annotated_event_names = set() + self.current_module_annotation_group_line_numbers = [] + + @check_all_messages(msgs) + def visit_module(self, node): + """ + Run all checks on a single module. + """ + self.check_module(node) + + def leave_module(self, _node): + self.current_module_annotated_event_names.clear() + self.current_module_annotation_group_line_numbers.clear() + + def check_annotation_group(self, search, annotations, node): + """ + Perform checks on a single annotation group. + """ + if not annotations: + return + + event_type = "" + event_name = "" + event_data = "" + event_description = "" + line_number = None + for annotation in annotations: + if line_number is None: + line_number = annotation["line_number"] + self.current_module_annotation_group_line_numbers.append(line_number) + if annotation["annotation_token"] == ".. event_type:": + event_type = annotation["annotation_data"] + elif annotation["annotation_token"] == ".. event_name:": + event_name = annotation["annotation_data"] + self.current_module_annotated_event_names.add(event_name) + elif annotation["annotation_token"] == ".. event_data:": + event_data = annotation["annotation_data"] + elif annotation["annotation_token"] == ".. event_description:": + event_description = annotation["annotation_data"] + elif annotation["annotation_token"] == ".. event_status:": + event_status = annotation["annotation_data"] + + if not event_type: + self.add_message( + self.NO_TYPE_MESSAGE_ID, + node=node, + line=line_number, + ) + + if not event_name: + self.add_message( + self.NO_NAME_MESSAGE_ID, + args=(event_type,), + node=node, + line=line_number, + ) + + if not event_data: + self.add_message( + self.NO_DATA_MESSAGE_ID, + args=(event_type,), + node=node, + line=line_number, + ) + + if not event_description: + self.add_message( + self.NO_DESCRIPTION_MESSAGE_ID, + args=(event_type,), + node=node, + line=line_number, + ) + + if not event_status: + self.add_message( + self.NO_STATUS_MESSAGE_ID, + args=(event_type,), + node=node, + line=line_number, + ) + + + @utils.check_messages(MISSING_ANNOTATION) + def visit_call(self, node): + """ + Check for missing annotations. + """ + if self.is_annotation_missing(node): + self.add_message( + self.MISSING_ANNOTATION, + node=node, + ) + + def is_annotation_missing(self, node): + """ + Check whether the node corresponds to a toggle instance creation. if yes, check that it is annotated. + """ + if ( + not isinstance(node.func, Name) + or node.func.name not in self.EVENT_CLASS_NAMES + ): + return False + + if not self.current_module_annotation_group_line_numbers: + # There are no annotations left + return True + + annotation_line_number = self.current_module_annotation_group_line_numbers[0] + if annotation_line_number > node.tolineno: + # The next annotation is located after the current node + return True + self.current_module_annotation_group_line_numbers.pop(0) + + # Check literal event name arguments + if node.args and isinstance(node.args[0], Const) and isinstance(node.args[0].value, str): + event_name = node.args[0].value + if event_name not in self.current_module_annotated_event_names: + return True + return False From e60eb115b43b66c84f9a178d3d1153147489afe6 Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Fri, 10 Jan 2025 14:55:25 +0100 Subject: [PATCH 03/10] refactor: drop `.. event_status` unnecessary annotation --- .../events_annotation/events_annotation_check.py | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/edx_lint/pylint/events_annotation/events_annotation_check.py b/edx_lint/pylint/events_annotation/events_annotation_check.py index 04048bf..9396c0b 100644 --- a/edx_lint/pylint/events_annotation/events_annotation_check.py +++ b/edx_lint/pylint/events_annotation/events_annotation_check.py @@ -112,8 +112,6 @@ def check_annotation_group(self, search, annotations, node): event_data = annotation["annotation_data"] elif annotation["annotation_token"] == ".. event_description:": event_description = annotation["annotation_data"] - elif annotation["annotation_token"] == ".. event_status:": - event_status = annotation["annotation_data"] if not event_type: self.add_message( @@ -146,15 +144,6 @@ def check_annotation_group(self, search, annotations, node): line=line_number, ) - if not event_status: - self.add_message( - self.NO_STATUS_MESSAGE_ID, - args=(event_type,), - node=node, - line=line_number, - ) - - @utils.check_messages(MISSING_ANNOTATION) def visit_call(self, node): """ From 2c6735ed68988521bf63a03024eaacaf9538df7a Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Fri, 10 Jan 2025 14:55:54 +0100 Subject: [PATCH 04/10] refactor: rename events_annotations.yaml to the correct config filename --- edx_lint/pylint/annotations_check.py | 2 +- edx_lint/pylint/events_annotation/events_annotation_check.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/edx_lint/pylint/annotations_check.py b/edx_lint/pylint/annotations_check.py index 9af40c9..b23d66d 100644 --- a/edx_lint/pylint/annotations_check.py +++ b/edx_lint/pylint/annotations_check.py @@ -296,7 +296,7 @@ class CodeAnnotationChecker(AnnotationBaseChecker): When creating a new annotation configuration, its filename should be added to CodeAnnotationChecker.CONFIG_FILENAMES (see AnnotationBaseChecker docs). """ - CONFIG_FILENAMES = ["feature_toggle_annotations.yaml", "setting_annotations.yaml", "events_annotations.yaml"] + CONFIG_FILENAMES = ["feature_toggle_annotations.yaml", "setting_annotations.yaml"] name = "code-annotations" msgs = { ("E%d%d" % (BASE_ID, index + 50)): ( diff --git a/edx_lint/pylint/events_annotation/events_annotation_check.py b/edx_lint/pylint/events_annotation/events_annotation_check.py index 9396c0b..a869099 100644 --- a/edx_lint/pylint/events_annotation/events_annotation_check.py +++ b/edx_lint/pylint/events_annotation/events_annotation_check.py @@ -20,7 +20,7 @@ class EventsAnnotationChecker(AnnotationBaseChecker): Perform checks on events annotations. """ - CONFIG_FILENAMES = ["events_annotations.yaml"] + CONFIG_FILENAMES = ["openedx_events_annotations.yaml"] __implements__ = (IAstroidChecker,) From 3309c2b208d7e1db1b4636817ed90aba125c2489 Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Fri, 10 Jan 2025 15:14:46 +0100 Subject: [PATCH 05/10] refactor: address quality errors --- .../pylint/events_annotation/events_annotation_check.py | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/edx_lint/pylint/events_annotation/events_annotation_check.py b/edx_lint/pylint/events_annotation/events_annotation_check.py index a869099..81b1be1 100644 --- a/edx_lint/pylint/events_annotation/events_annotation_check.py +++ b/edx_lint/pylint/events_annotation/events_annotation_check.py @@ -2,12 +2,12 @@ Pylint plugin: checks that Open edX Events are properly annotated. """ -from pylint.interfaces import IAstroidChecker +from astroid.nodes.node_classes import Const, Name from pylint.checkers import utils -from edx_lint.pylint.common import BASE_ID from edx_lint.pylint.annotations_check import AnnotationBaseChecker, check_all_messages -from astroid.node_classes import Const, Name +from edx_lint.pylint.common import BASE_ID + def register_checkers(linter): """ @@ -15,6 +15,7 @@ def register_checkers(linter): """ linter.register_checker(EventsAnnotationChecker(linter)) + class EventsAnnotationChecker(AnnotationBaseChecker): """ Perform checks on events annotations. @@ -22,8 +23,6 @@ class EventsAnnotationChecker(AnnotationBaseChecker): CONFIG_FILENAMES = ["openedx_events_annotations.yaml"] - __implements__ = (IAstroidChecker,) - name = "events-annotations" NO_TYPE_MESSAGE_ID = "event-no-type" From 8d1079e42b1f23cc25f7c2e61b1f3b7a1b707778 Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Fri, 10 Jan 2025 15:18:46 +0100 Subject: [PATCH 06/10] refactor: address quality errors --- edx_lint/pylint/events_annotation/events_annotation_check.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/edx_lint/pylint/events_annotation/events_annotation_check.py b/edx_lint/pylint/events_annotation/events_annotation_check.py index 81b1be1..0dc231c 100644 --- a/edx_lint/pylint/events_annotation/events_annotation_check.py +++ b/edx_lint/pylint/events_annotation/events_annotation_check.py @@ -143,7 +143,7 @@ def check_annotation_group(self, search, annotations, node): line=line_number, ) - @utils.check_messages(MISSING_ANNOTATION) + @utils.only_required_for_messages(MISSING_ANNOTATION) def visit_call(self, node): """ Check for missing annotations. From d1adeda3eda7a18b8c98c400964ad7acf1727fd6 Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Thu, 16 Jan 2025 12:16:59 +0100 Subject: [PATCH 07/10] refactor!: check whether event type and data matches definition for group annotations --- .../events_annotation_check.py | 81 ++++++++++++------- 1 file changed, 51 insertions(+), 30 deletions(-) diff --git a/edx_lint/pylint/events_annotation/events_annotation_check.py b/edx_lint/pylint/events_annotation/events_annotation_check.py index 0dc231c..10299a0 100644 --- a/edx_lint/pylint/events_annotation/events_annotation_check.py +++ b/edx_lint/pylint/events_annotation/events_annotation_check.py @@ -30,40 +30,35 @@ class EventsAnnotationChecker(AnnotationBaseChecker): NO_DATA_MESSAGE_ID = "event-no-data" NO_STATUS_MESSAGE_ID = "event-no-status" NO_DESCRIPTION_MESSAGE_ID = "event-empty-description" - MISSING_ANNOTATION = "event-missing-annotation" + MISSING_OR_INCORRECT_ANNOTATION = "missing-or-incorrect-annotation" msgs = { ("E%d80" % BASE_ID): ( - "event annotation has no type", + "Event type must be present and be the first annotation", NO_TYPE_MESSAGE_ID, - "Events annotations type must be present and be the first annotation", + "event type must be present and be the first annotation", ), ("E%d81" % BASE_ID): ( - "event annotation (%s) has no name", + "Event annotation (%s) has no name", NO_NAME_MESSAGE_ID, - "Events annotations name must be present", + "Event annotations must include a name", ), ("E%d82" % BASE_ID): ( - "event annotation (%s) has no data argument", + "Event annotation (%s) has no data", NO_DATA_MESSAGE_ID, - "Events annotations must include data argument", - ), - ("E%d83" % BASE_ID): ( - "event annotation (%s) has no status", - NO_STATUS_MESSAGE_ID, - "Events annotations must include the status of event", + "event annotations must include a data", ), ("E%d84" % BASE_ID): ( - "event annotation (%s) does not have a description", + "Event annotation (%s) has no description", NO_DESCRIPTION_MESSAGE_ID, "Events annotations must include a short description", ), ("E%d85" % BASE_ID): ( - "missing event annotation", - MISSING_ANNOTATION, + "Event annotation is missing or incorrect", + MISSING_OR_INCORRECT_ANNOTATION, ( "When an Open edX event object is created, a corresponding annotation must be present above in the" - " same module and with a matching name", + " same module and with a matching type", ) ), } @@ -72,8 +67,10 @@ class EventsAnnotationChecker(AnnotationBaseChecker): def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) - self.current_module_annotated_event_names = set() + self.current_module_annotated_event_types = [] + self.current_module_event_data = [] self.current_module_annotation_group_line_numbers = [] + self.current_module_annotation_group_map = {} @check_all_messages(msgs) def visit_module(self, node): @@ -83,8 +80,8 @@ def visit_module(self, node): self.check_module(node) def leave_module(self, _node): - self.current_module_annotated_event_names.clear() self.current_module_annotation_group_line_numbers.clear() + self.current_module_annotation_group_map.clear() def check_annotation_group(self, search, annotations, node): """ @@ -102,15 +99,17 @@ def check_annotation_group(self, search, annotations, node): if line_number is None: line_number = annotation["line_number"] self.current_module_annotation_group_line_numbers.append(line_number) + self.current_module_annotation_group_map[line_number] = () if annotation["annotation_token"] == ".. event_type:": event_type = annotation["annotation_data"] elif annotation["annotation_token"] == ".. event_name:": event_name = annotation["annotation_data"] - self.current_module_annotated_event_names.add(event_name) elif annotation["annotation_token"] == ".. event_data:": event_data = annotation["annotation_data"] elif annotation["annotation_token"] == ".. event_description:": event_description = annotation["annotation_data"] + if event_type and event_data: + self.current_module_annotation_group_map[line_number] = (event_type, event_data,) if not event_type: self.add_message( @@ -143,20 +142,29 @@ def check_annotation_group(self, search, annotations, node): line=line_number, ) - @utils.only_required_for_messages(MISSING_ANNOTATION) + @utils.only_required_for_messages(MISSING_OR_INCORRECT_ANNOTATION) def visit_call(self, node): """ Check for missing annotations. """ - if self.is_annotation_missing(node): + if self._is_annotation_missing_or_incorrect(node): self.add_message( - self.MISSING_ANNOTATION, + self.MISSING_OR_INCORRECT_ANNOTATION, node=node, ) - def is_annotation_missing(self, node): + def _is_annotation_missing_or_incorrect(self, node): """ - Check whether the node corresponds to a toggle instance creation. if yes, check that it is annotated. + Check if an annotation is missing or incorrect for an event. + + An annotation is considered missing when: + - The annotation is not present above the event object. + + An annotation is considered incorrect when: + - The annotation is present above the event object but the type of the annotation does + not match the type of the event object. + - The annotation is present above the event object but the data of the annotation does + not match the data of the event object. """ if ( not isinstance(node.func, Name) @@ -172,11 +180,24 @@ def is_annotation_missing(self, node): if annotation_line_number > node.tolineno: # The next annotation is located after the current node return True - self.current_module_annotation_group_line_numbers.pop(0) + annotation_line_number = self.current_module_annotation_group_line_numbers.pop(0) + + current_annotation_group = self.current_module_annotation_group_map[annotation_line_number] + if not current_annotation_group: + # The annotation group with type and data for the line is empty, but should be caught by the annotation + # checks + return False + + current_event_type, current_event_data = current_annotation_group + # All event definitions have two keyword arguments, the first is the event type and the second is the + # event data. For example: + # OpenEdxPublicSignal( + # event_type="org.openedx.subdomain.action.emitted.v1", + # event_data={"my_data": MyEventData}, + # ) + node_event_type = node.keywords[0].value.value + node_event_data = node.keywords[1].value.items[0][1].name + if node_event_type != current_event_type or node_event_data != current_event_data: + return True - # Check literal event name arguments - if node.args and isinstance(node.args[0], Const) and isinstance(node.args[0].value, str): - event_name = node.args[0].value - if event_name not in self.current_module_annotated_event_names: - return True return False From dadaa925da30eac6fe2bac990424538701abe2f5 Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Thu, 16 Jan 2025 12:37:44 +0100 Subject: [PATCH 08/10] refactor: consider event name in linting strategy --- .../events_annotation_check.py | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/edx_lint/pylint/events_annotation/events_annotation_check.py b/edx_lint/pylint/events_annotation/events_annotation_check.py index 10299a0..87f6404 100644 --- a/edx_lint/pylint/events_annotation/events_annotation_check.py +++ b/edx_lint/pylint/events_annotation/events_annotation_check.py @@ -108,8 +108,8 @@ def check_annotation_group(self, search, annotations, node): event_data = annotation["annotation_data"] elif annotation["annotation_token"] == ".. event_description:": event_description = annotation["annotation_data"] - if event_type and event_data: - self.current_module_annotation_group_map[line_number] = (event_type, event_data,) + if event_type and event_data and event_name: + self.current_module_annotation_group_map[line_number] = (event_type, event_data, event_name,) if not event_type: self.add_message( @@ -184,20 +184,21 @@ def _is_annotation_missing_or_incorrect(self, node): current_annotation_group = self.current_module_annotation_group_map[annotation_line_number] if not current_annotation_group: - # The annotation group with type and data for the line is empty, but should be caught by the annotation - # checks + # The annotation group with type or data or name for the line is empty, but should be caught by the + # annotation checks return False - current_event_type, current_event_data = current_annotation_group + event_type, event_data, event_name = current_annotation_group # All event definitions have two keyword arguments, the first is the event type and the second is the - # event data. For example: - # OpenEdxPublicSignal( + # event data. It also has a name associated with it. For example: + # MyEvent = OpenEdxPublicSignal( # event_type="org.openedx.subdomain.action.emitted.v1", # event_data={"my_data": MyEventData}, # ) node_event_type = node.keywords[0].value.value node_event_data = node.keywords[1].value.items[0][1].name - if node_event_type != current_event_type or node_event_data != current_event_data: + node_event_name = node.parent.targets[0].name + if node_event_type != event_type or node_event_data != event_data or node_event_name != event_name: return True return False From e84bc862d4fb96fb87e0f17f057f1770b944abc9 Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Thu, 16 Jan 2025 12:46:32 +0100 Subject: [PATCH 09/10] refactor: address quality errors --- edx_lint/pylint/events_annotation/events_annotation_check.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/edx_lint/pylint/events_annotation/events_annotation_check.py b/edx_lint/pylint/events_annotation/events_annotation_check.py index 87f6404..c6306a7 100644 --- a/edx_lint/pylint/events_annotation/events_annotation_check.py +++ b/edx_lint/pylint/events_annotation/events_annotation_check.py @@ -2,7 +2,7 @@ Pylint plugin: checks that Open edX Events are properly annotated. """ -from astroid.nodes.node_classes import Const, Name +from astroid.nodes.node_classes import Name from pylint.checkers import utils from edx_lint.pylint.annotations_check import AnnotationBaseChecker, check_all_messages From c14e40c56efe0992a66d23f0d5314e9be30127a0 Mon Sep 17 00:00:00 2001 From: Maria Grimaldi Date: Wed, 22 Jan 2025 10:57:56 +0100 Subject: [PATCH 10/10] docs: update documentation for new release --- CHANGELOG.rst | 5 +++++ edx_lint/__init__.py | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 5de7206..234e7d5 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -13,6 +13,11 @@ Change Log Unreleased ~~~~~~~~~~ +5.5.0 - 2025-01-22 +~~~~~~~~~~~~~~~~~~ + +* Add inline code annotation linter for Open edX Events. + 5.4.1 - 2024-10-28 ~~~~~~~~~~~~~~~~~~ diff --git a/edx_lint/__init__.py b/edx_lint/__init__.py index cab2caa..f7cfbb9 100644 --- a/edx_lint/__init__.py +++ b/edx_lint/__init__.py @@ -2,4 +2,4 @@ edx_lint standardizes lint configuration and additional plugins for use in Open edX code. """ -__version__ = "5.4.1" +__version__ = "5.5.0"