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

mariajgrimaldi · 2025-01-13T10:57:08Z

Description:

This PR adds a custom linter for Open edX Filter classes docstrings to check whether new filters follow the specified format:

        Description:
        Filter used to modify the certificate rendering process.
        ... (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

If a class is missing at least one of the section, then a pylint error is raised:

You can see the linter integrated into the repo in this PR: openedx/openedx-filters#249

I'm open to moving this directly into the openedx-filters repo if needed, considering that the linter is kind of specific to that repository.

Merge checklist:

All reviewers approved
CI build is green
If adding new checks, followed how_tos/0001-adding-new-checks.rst
Changelog record added (if needed)
Documentation updated (not only docstrings) (if needed)
Commits are squashed

openedx-webhooks · 2025-01-13T10:57:15Z

Thanks for the pull request, @mariajgrimaldi!

This repository is currently maintained by @openedx/axim-engineering.

Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review.

🔘 Get product approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
- This process (including the steps you'll need to take) is documented here.
If it doesn't, simply proceed with the next step.

🔘 Provide context

To help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:

Dependencies

This PR must be merged before / after / at the same time as ...
Blockers

This PR is waiting for OEP-1234 to be accepted.
Timeline information

This PR must be merged by XX date because ...
Partner information

This is for a course on edx.org.
Supporting documentation
Relevant Open edX discussion forum threads

🔘 Get a green build

If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green.

Where can I find more information?

If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:

When can I expect my changes to be merged?

Our goal is to get community contributions seen and reviewed as efficiently as possible.

However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:

The size and impact of the changes that it introduces
The need for product review
Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

sarina · 2025-01-13T23:39:52Z

I'm open to moving this directly into the openedx-filters repo if needed, considering that the linter is kind of specific to that repository.

I pinged the Axim eng team in slack to see if anyone has an opinion on this - I don't, but I'm not a primary maintainer of this repo.

mariajgrimaldi · 2025-01-14T09:59:13Z

Thank you, @sarina!

I'll tag @openedx/axim-engineering here as the owners according to the catalog-info.yaml.

bmtcril · 2025-01-14T14:09:21Z

Hi @mariajgrimaldi ! My memory isn't great on this, but I think that:

We want to define filters in openedx-filters
But some filters may be defined elsewhere (ex: filters only used repositories that aren't in the openedx org?)

If that's the case I think this is a good place for that check. If it's a requirement that filters only be defined in openedx-filters then I'd expect this check to live there. Otherwise I'm 👍 on this PR, just want to make sure it's in the right place.

edx_lint/pylint/filters_docstring/filters_docstring_check.py

Alec4r · 2025-01-14T21:42:09Z

edx_lint/pylint/filters_docstring/filters_docstring_check.py

+        """
+        required_sections = [
+            (r"Purpose:\s*.*\n", self.DOCSTRING_MISSING_PURPOSE),
+            (r"Filter Type:\s*.*\n", self.DOCSTRING_MISSING_TYPE),


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?

Yes, we have a format for the filter_type attribute (see ADR-0004), but even though, there are a lot of cases to consider. A good middle ground to still allow flexibility could be to check that the Filter Type from the docstring matches the OpenEdxPublicFilter.filter_type, so we know that docstrings contain accurate information. I can try applying this suggestion to openedx-events as well.

Alec4r · 2025-01-14T21:49:22Z

edx_lint/pylint/filters_docstring/filters_docstring_check.py

+        OpenEdxPublicFilter class itself.
+
+        """
+        if not node.is_subtype_of("openedx_filters.tooling.OpenEdxPublicFilter"):


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

With only the first condition, the linter was reviewing OpenEdxPublicFilter as well, so I added the second condition to avoid that behavior. I'm doing more tests to see why I was getting that behavior. I'll let you know what I find :)

EDIT: OpenEdxPublicFilter can be its own subtype

>>> node.is_subtype_of("openedx_filters.tooling.OpenEdxPublicFilter") True >>> node.name 'OpenEdxPublicFilter'

That's why we need the 2nd condition. I implemented your suggestion to take advantage of the short-circuit behavior.

mariajgrimaldi · 2025-01-15T18:46:06Z

edx_lint/pylint/filters_docstring/filters_docstring_check.py

+
+        If the filter type is missing or incorrect, return the error message. Otherwise, return.
+        """
+        filter_type = node.locals["filter_type"][0].statement().value.value


note to self: I think I should do some error handling here (like in https://github.com/openedx/edx-lint/blob/master/edx_lint/pylint/annotations_check.py#L515?).

I think that makes sense, otherwise I'm 👍 on this

I implemented our suggestion here: 7455a5e. Thank you!

mariajgrimaldi · 2025-01-22T08:46:23Z

@bmtcril: Thanks for the insight! So yes, the goal is for filters and events to implement reusable ones in each of the openedx repositories, making them easy for the community to find and use. Although it's not restricted to that, implementing a filter or event specific to an organization in its own repository is encouraged.

bmtcril · 2025-01-22T13:40:08Z

I think it just needs a rebase and version bump to be good to go! 🚀

mariajgrimaldi · 2025-01-24T10:56:02Z

@bmtcril: thank you for the reviews! This is ready for merging when you see fit :)

bmtcril · 2025-01-24T18:40:23Z

All set and released!

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Jan 13, 2025

mariajgrimaldi mentioned this pull request Jan 13, 2025

feat: [FC-0074] use linter for openedx-filters classes definitions openedx/openedx-filters#249

Merged

15 tasks

mariajgrimaldi changed the title ~~feat: add linter for openedx-filters classes definitions~~ feat: [FC-0074] add linter for openedx-filters classes definitions Jan 13, 2025

mariajgrimaldi added the FC Relates to an Axim Funded Contribution project label Jan 13, 2025

mariajgrimaldi changed the title ~~feat: [FC-0074] add linter for openedx-filters classes definitions~~ feat: [FC-0074] add linter for Open edX Filters classes definitions Jan 13, 2025

mariajgrimaldi marked this pull request as ready for review January 13, 2025 12:40

mariajgrimaldi requested a review from sarina January 13, 2025 12:40

Alec4r reviewed Jan 14, 2025

View reviewed changes

edx_lint/pylint/filters_docstring/filters_docstring_check.py Outdated Show resolved Hide resolved

Alec4r reviewed Jan 14, 2025

View reviewed changes

mariajgrimaldi force-pushed the MJG/filter-docstring-linter branch from 5d4a122 to b0c3aec Compare January 15, 2025 18:37

mariajgrimaldi commented Jan 15, 2025

View reviewed changes

Alec4r approved these changes Jan 16, 2025

View reviewed changes

bmtcril approved these changes Jan 22, 2025

View reviewed changes

mariajgrimaldi added 10 commits January 24, 2025 11:53

feat: add linter for openedx-filters classes definitions

4128909

refactor: address quality errors

b0d2c02

refactor: drop filter prefix from variables names

7a05454

refactor: add additional details for error message

9c6bd81

refactor: skip OpenEdxPublicFilter base class

6b95646

refactor: use purpose only instead of description

d88857b

refactor: make checks more specific with separation of concerns

b990554

refactor: combine conditions in single statement

9659232

refactor: address quality errors

bacec37

refactor: add error handling when getting filter_type from class

ae8fbdf

docs: update docstrings for new release

46ac9a3

mariajgrimaldi force-pushed the MJG/filter-docstring-linter branch from 7455a5e to 46ac9a3 Compare January 24, 2025 10:53

bmtcril merged commit b1bc0ea into openedx:master Jan 24, 2025
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

mariajgrimaldi commented Jan 13, 2025 •

edited

Loading

openedx-webhooks commented Jan 13, 2025 •

edited

Loading

🔘 Get product approval

🔘 Provide context

🔘 Get a green build

Where can I find more information?

When can I expect my changes to be merged?

sarina commented Jan 13, 2025

mariajgrimaldi commented Jan 14, 2025

bmtcril commented Jan 14, 2025

Alec4r Jan 14, 2025

mariajgrimaldi Jan 15, 2025

Alec4r Jan 14, 2025

mariajgrimaldi Jan 15, 2025 •

edited

Loading

mariajgrimaldi Jan 15, 2025 •

edited

Loading

bmtcril Jan 16, 2025

mariajgrimaldi Jan 22, 2025

mariajgrimaldi commented Jan 22, 2025

bmtcril commented Jan 22, 2025

mariajgrimaldi commented Jan 24, 2025

bmtcril commented Jan 24, 2025

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

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

Conversation

mariajgrimaldi commented Jan 13, 2025 • edited Loading

openedx-webhooks commented Jan 13, 2025 • edited Loading

🔘 Get product approval

🔘 Provide context

🔘 Get a green build

Where can I find more information?

When can I expect my changes to be merged?

sarina commented Jan 13, 2025

mariajgrimaldi commented Jan 14, 2025

bmtcril commented Jan 14, 2025

Alec4r Jan 14, 2025

Choose a reason for hiding this comment

mariajgrimaldi Jan 15, 2025

Choose a reason for hiding this comment

Alec4r Jan 14, 2025

Choose a reason for hiding this comment

mariajgrimaldi Jan 15, 2025 • edited Loading

Choose a reason for hiding this comment

mariajgrimaldi Jan 15, 2025 • edited Loading

Choose a reason for hiding this comment

bmtcril Jan 16, 2025

Choose a reason for hiding this comment

mariajgrimaldi Jan 22, 2025

Choose a reason for hiding this comment

mariajgrimaldi commented Jan 22, 2025

bmtcril commented Jan 22, 2025

mariajgrimaldi commented Jan 24, 2025

bmtcril commented Jan 24, 2025

mariajgrimaldi commented Jan 13, 2025 •

edited

Loading

openedx-webhooks commented Jan 13, 2025 •

edited

Loading

mariajgrimaldi Jan 15, 2025 •

edited

Loading

mariajgrimaldi Jan 15, 2025 •

edited

Loading