Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: [FC-0074] add linter for Open edX Filters classes definitions #480

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions edx_lint/pylint/filters_docstring/__init__.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
"""
edx_lint filters_docstring module (optional plugin for filters docstrings).
Add this to your pylintrc::
load-plugins=edx_lint.pylint.filters_docstring
"""

from .filters_docstring_check import register_checkers

register = register_checkers
117 changes: 117 additions & 0 deletions edx_lint/pylint/filters_docstring/filters_docstring_check.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,117 @@
"""
Pylint checker for the format of the docstrings of filters.

A filter's docstring should have the following structure:

1. Description: Any non-empty text followed by a blank line.
2. Filter Type: A line that starts with "Filter Type:".
3. Trigger: A line that starts with "Trigger:".
"""

import re

from pylint.checkers import BaseChecker, utils

from edx_lint.pylint.common import BASE_ID


def register_checkers(linter):
"""
Register checkers.
"""
linter.register_checker(FiltersDocstringFormatChecker(linter))


class FiltersDocstringFormatChecker(BaseChecker):
"""Pylint checker for the format of the docstrings of filters."""

name = "docstring-format-checker"

DOCSTRING_MISSING_PURPOSE = "filter-docstring-missing-purpose"
DOCSTRING_MISSING_TYPE = "filter-docstring-missing-type"
DOCSTRING_MISSING_TRIGGER = "filter-docstring-missing-trigger"

msgs = {
("E%d91" % BASE_ID): (
"Filter's (%s) docstring is missing the required purpose section or is badly formatted",
DOCSTRING_MISSING_PURPOSE,
"filters docstring is missing the required purpose section or is badly formatted",
),
("E%d92" % BASE_ID): (
"Filter's (%s) docstring is missing the required type section or is badly formatted",
DOCSTRING_MISSING_TYPE,
"filters docstring is missing the required type section or is badly formatted",
),
("E%d93" % BASE_ID): (
"Filter's (%s) docstring is missing the required trigger section or is badly formatted",
DOCSTRING_MISSING_TRIGGER,
"filters docstring is missing the required trigger section or is badly formatted",
),
}

options = ()

@utils.only_required_for_messages(
DOCSTRING_MISSING_PURPOSE,
DOCSTRING_MISSING_TYPE,
DOCSTRING_MISSING_TRIGGER,
)
def visit_classdef(self, node):
"""
Visit a class definition and check its docstring.

If the class is a subclass of OpenEdxPublicFilter, check the format of its docstring. Skip the
OpenEdxPublicFilter class itself.

"""
if not node.is_subtype_of("openedx_filters.tooling.OpenEdxPublicFilter"):
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This checker currently validates both whether the class is a subclass of OpenEdxPublicFilter and whether the class name is exactly OpenEdxPublicFilter but this second validation is redundant 'cause by definition, the OpenEdxPublicFilter class cannot be its own subclass, maybe we could do something like this:

if not node.is_subtype_of("openedx_filters.tooling.OpenEdxPublicFilter") or node.name == "OpenEdxPublicFilter":
    return

return

if node.name == "OpenEdxPublicFilter":
return

docstring = node.doc_node.value if node.doc_node else ""
if not (error_messages := self._check_docstring_format(docstring)):
return
for error_message in error_messages:
self.add_message(error_message, node=node, args=(node.name,))

def _check_docstring_format(self, docstring):
"""
Check the format of the docstring for errors and return a list of error messages.

The docstring should have the following structure:
1. Description: Any non-empty text followed by a blank line.
2. Filter Type: A line that starts with "Filter Type:".
3. Trigger: A line that starts with "Trigger:".

For example:

```
Description:
Filter used to modify the certificate rendering process.
Comment on lines +84 to +92
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the required_sections, you're expecting 'Purpose:', but in this example, it is referred to as 'Description:'. Could you update it for consistency?


... (more description)

Filter Type:
org.openedx.learning.certificate.render.started.v1

Trigger:
- Repository: openedx/edx-platform
- Path: lms/djangoapps/certificates/views/webview.py
- Function or Method: render_html_view
```
"""
required_sections = [
(r"Purpose:\s*.*\n", self.DOCSTRING_MISSING_PURPOSE),
(r"Filter Type:\s*.*\n", self.DOCSTRING_MISSING_TYPE),
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are currently just validating that it's not empty, shouldn't we make this regex more robust to ensure it matches a valid type? Or do we not have a defined set of valid types yet?

(
r"Trigger:\s*(NA|-\s*Repository:\s*[^\n]+\s*-\s*Path:\s*[^\n]+\s*-\s*Function\s*or\s*Method:\s*[^\n]+)",
self.DOCSTRING_MISSING_TRIGGER,
),
]
error_messages = []
for pattern, error_message in required_sections:
if not re.search(pattern, docstring, re.MULTILINE):
error_messages.append(error_message)
return error_messages
Loading