Enforce quality checks and type hints, improve quality and typing by bradenmacdonald · Pull Request #73 · openedx/openedx-core

bradenmacdonald · 2023-08-16T21:04:43Z

As it turns out, the CI checks for this repo were not running pylint or mypy, and there were a ton of lint errors and typing bugs.

This repo's packages also didn't have any py.typed files, so whatever type hints they declared were being ignored by other code like edx-platform code that uses this.

This PR:

configures CI to run quality checks, including using mypy to do static type checking of the code.
Adds more type hints, both to make code visible to mypy and to improve its quality
fixes all the lint issues found
fixes all the type hint issues found

A few of these issues automatically identified are clearly bugs, which are now fixed :)

Private-ref: FAL-3476

openedx-webhooks · 2023-08-16T21:04:47Z

Thanks for the pull request, @bradenmacdonald! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

supporting documentation
Open edX discussion forum threads
timeline information ("this must be merged by XX date", and why that is)
partner information ("this is a course on edx.org")
any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

bradenmacdonald · 2023-08-16T21:05:32Z

openedx_learning/core/publishing/api.py

            message=message,
            published_at=published_at,
-            published_by=published_by,
+            published_by_id=published_by,


This was a bug: the function accepted an int but assigned it to a User model attribute.

bradenmacdonald · 2023-08-16T21:05:44Z

openedx_learning/core/publishing/api.py

    Check whether a LearningPackage with a particular key exists.
    """
-    LearningPackage.objects.filter(key=key).exists()
+    return LearningPackage.objects.filter(key=key).exists()


This was a bug - missing return statement.

bradenmacdonald · 2023-08-23T18:08:11Z

openedx_learning/core/components/admin.py

        )


-def is_displayable_text(mime_type):


This function was no longer used, after a prior refactor.

bradenmacdonald · 2023-08-23T18:09:38Z

openedx_learning/core/contents/api.py

+    """
+    Create a new TextContent instance for the given RawContent.
+    """
+    text = codecs.decode(raw_content.file.open().read(), encoding)


This was a bug (I assume): the API accepted an encoding parameter but ignored it, and always used "utf-8-sig"

bradenmacdonald · 2023-08-23T18:10:13Z

openedx_learning/core/contents/api.py

        )
        if rc_created or not hasattr(raw_content, "text_content"):
-            text = codecs.decode(data_bytes, "utf-8-sig")
+            text = codecs.decode(data_bytes, encoding)


Same here: encoding parameter was silently ignored.

bradenmacdonald · 2023-08-23T18:13:50Z

openedx_learning/lib/fields.py



-def create_hash_digest(data_bytes):
+def create_hash_digest(data_bytes: bytes) -> str:


With a type annotation, this argument could be renamed from data_bytes to data: bytes which is much more clear (data_bytes could be a size in bytes, for example). But the name data_bytes is already used by create_raw_content in the public API so I'm not sure if it's OK to change.

The only place it could be used is edx-platform, so I think it's okay to change if it's not already referenced there. I also don't think it's that important to do so though.

Yeah honestly this is a very minor thing :p

bradenmacdonald · 2023-08-23T18:15:53Z

openedx_tagging/core/tagging/api.py

    taxonomy = Taxonomy(
        name=name,
-        description=description,
+        description=description or "",


Per Django convention, NULL/None should not be used for text fields.

bradenmacdonald · 2023-08-23T18:18:13Z

openedx_tagging/core/tagging/import_export/exceptions.py

-    def __init__(self, tag: dict, format: str, message: str, **kargs):
-        self.tag = tag
+    def __init__(self, tag: dict | None, format: str, message: str, **kargs):
+        super().__init__(tag)


These exception classes are quite messy and could use a refactor. Once you add type hints, the problems become more obvious. In particular, none of them were calling super().__init__(), and the purpose of the tag field is unclear. Some have hard-coded messages and some let you override the message. For now I just made the minimal changes to get type checks passing.

bradenmacdonald · 2023-08-23T18:19:24Z

openedx_tagging/core/tagging/import_export/parsers.py

            tags_data = json.load(file)
        except json.JSONDecodeError as error:
-            return None, [
+            return [], [


This was returning (None, list) in a clear violation of the declared return type (list, list)

bradenmacdonald · 2023-08-23T18:19:59Z

openedx_tagging/core/tagging/import_export/parsers.py

+        errors = cls._verify_header(list(header_fields or []))
        if errors:
-            return None, errors
+            return [], errors


Same here, invalid return of None

bradenmacdonald · 2023-08-23T18:23:51Z

openedx_tagging/core/tagging/import_export/api.py

-    return task.status
+    if task is None:
+        raise ValueError("No import task was created yet.")
+    return TagImportTaskState(task.status)


This was a bug: the typing declared that it returned enum instances, but it was returning the enum values which are just plain strings. Also it wasn't handling the case where there was no "last import task".

bradenmacdonald · 2023-08-23T18:36:10Z

openedx_tagging/core/tagging/models/base.py

        ),
    )
    description = MultiCollationTextField(
-        null=True,


Django convention is not not use NULL for text fields, unless there's a very good reason, which doesn't seem to be the case here.

bradenmacdonald · 2023-08-23T21:02:47Z

tests/openedx_tagging/core/tagging/test_system_defined_models.py

-        ) == expected
+    # FIXME: something is wrong with this test case. It's setting the string
+    # "tag_id" as the primary key (integer) of the Tag instance, and it mentions
+    # "parent validation" but there is nothing to do with parents here.


I'm not sure what was going on with this test case. It was failing and I don't know what it's even trying to do, so for now I commented it out.

@ChrisChV Could you please check this when you have time?

bradenmacdonald · 2023-08-23T21:03:27Z

tests/openedx_tagging/core/tagging/test_system_defined_models.py

-            object_id="id",
-            tag=tag,
-        )
-        self.language_taxonomy.validate_object_tag(


This function just returns a bool so this test was having no effect. I commented it out because again I'm not sure what it's supposed to be doing.

@ChrisChV Same here.

rpenido

Hi @bradenmacdonald! Awesome work here.

I just pointed out some nits, but feel free to push back if you want to!

We are working a lot with this repository, so we should merge this ASAP to avoid a rebase hell.

Question:

Is there a chance that we break edx-platform (if the version is not pinned) because we forced the types here, and there is a wrong call on edx-platform side (or another package that imports this package)?

PS: Running make quality I got a warning on the console:

pylint: Command line or configuration file:1: UserWarning: Specifying exception names in the overgeneral-exceptions option without module name is deprecated and support for it will be removed in pylint 3.0. Use fully qualified name (maybe 'builtins.Exception' ?) instead.

Changing the pylintrc as suggested in the warning removed it.

[EXCEPTIONS]
overgeneral-exceptions = builtins.Exception

openedx_tagging/core/tagging/models/base.py

openedx_learning/core/components/admin.py

openedx_learning/lib/collations.py

openedx_learning/lib/fields.py

openedx_learning/rest_api/v1/components.py

openedx_tagging/core/tagging/models/system_defined.py

bradenmacdonald · 2023-08-24T00:09:35Z

@rpenido Thanks for the great review! (and fast!) I agree that we should merge this ASAP because of the large number of possible conflicts.

PS: Running make quality I got a warning on the console... Changing the pylintrc as suggested in the warning removed it.

The pylintrc is managed by edx-lint, so they'll probably fix it "upstream" at some point. We don't need to fix it separately in this repo.

Is there a chance that we break edx-platform (if the version is not pinned) because we forced the types here, and there is a wrong call on edx-platform side (or another package that imports this package)?

Nope. Python ignores the types at runtime, and on the edx-platform side they're not even being checked yet. Actually once this PR is merged, I'm going to modify edx-platform so that they are checked.

Is there something that I can check on the django-rulesPR? I'm not sure how to reproduce what are you experiencing.

Sorry, I didn't mention that. You need to edit openedx_tagging/core/tagging/rules.py to remove the # type: ignore from the line where we import rules, and then if you run mypy you'll see the error I mentioned.

bradenmacdonald · 2023-08-24T00:12:51Z

@ormsbee Since Jill is on leave, would you be able to approve merging this PR?

ormsbee

Looks good to me. I realize there are some follow-up items from this (like the test), but it's probably better to merge this ASAP given it's size and breadth.

Thanks @bradenmacdonald. This is one of those things that I really wanted to see happen but didn't get around to. It warms my heart to see this sort of thing revealing bugs.

openedx-webhooks · 2023-08-24T16:48:20Z

@bradenmacdonald 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Aug 16, 2023

bradenmacdonald commented Aug 16, 2023

View reviewed changes

feat: enable type hints and mypy checking for this repository

a157711

bradenmacdonald force-pushed the braden/type-hints branch from 45c4435 to e73081e Compare August 16, 2023 23:13

bradenmacdonald changed the title ~~Use mypy to check type hints (WIP)~~ Enforce quality checks and type hints, improve quality and typing Aug 16, 2023

bradenmacdonald added 2 commits August 17, 2023 11:53

fix: pylint issues

1cb67ac

fix: improve type hints of openedx_learning and fix identified bugs

6886014

bradenmacdonald force-pushed the braden/type-hints branch from 1aabddf to 6886014 Compare August 17, 2023 19:19

bradenmacdonald added 8 commits August 17, 2023 12:45

fix: typing issues in tagging code

191cddb

fix: typing issues in tagging code

852b388

fix: typing issues in tagging code

59a80b0

fix: typing issues in tagging code

5729a19

fix: typing issues in tagging code

a710cc0

chore: update with latest master

d46f720

feat: opt more code into type checking

ca706b2

feat: type check the test suite

75b429b

bradenmacdonald commented Aug 23, 2023

View reviewed changes

chore: add migration to prevent taxonomy description from being null

d14b590

bradenmacdonald commented Aug 23, 2023

View reviewed changes

fix: duplicate module error

2079cc0

bradenmacdonald commented Aug 23, 2023

View reviewed changes

bradenmacdonald force-pushed the braden/type-hints branch from 6e2dfdc to e9fabfc Compare August 23, 2023 18:54

bradenmacdonald added 2 commits August 23, 2023 11:57

chore: fix some mypy warnings that only appeared on CI

826553d

feat: add even more type annotations

87d5840

bradenmacdonald force-pushed the braden/type-hints branch 2 times, most recently from 647babd to 0697182 Compare August 23, 2023 20:01

fix: PEP8 / pycodestyle issues

ee19093

bradenmacdonald force-pushed the braden/type-hints branch from 0697182 to ee19093 Compare August 23, 2023 20:04

bradenmacdonald added 3 commits August 23, 2023 13:09

fix: run isort

6148244

fix: invalid descriptions in test fixture

77b9ff5

fix: failing test cases

488dac4

bradenmacdonald force-pushed the braden/type-hints branch from b17a5fe to 488dac4 Compare August 23, 2023 21:00

bradenmacdonald commented Aug 23, 2023

View reviewed changes

rpenido reviewed Aug 23, 2023

View reviewed changes

openedx_tagging/core/tagging/models/system_defined.py Outdated Show resolved Hide resolved

bradenmacdonald added 3 commits August 23, 2023 16:56

fix: address review comments

57fb09d

chore: be more explicit about what type error we're ignoring

d8e04af

feat: add a few more missing type annotations

b7f5a84

ormsbee approved these changes Aug 24, 2023

View reviewed changes

bradenmacdonald merged commit 01185f3 into main Aug 24, 2023

bradenmacdonald deleted the braden/type-hints branch August 24, 2023 16:48

bradenmacdonald mentioned this pull request Aug 29, 2023

[Tagging] Enforce typing and encapsulation for tagging APIs openedx/modular-learning#83

Open

ormsbee mentioned this pull request Mar 24, 2025

add vscode pytest settings open-craft/openedx-learning#19

Closed

ormsbee mentioned this pull request Sep 4, 2025

version bump 0.28.0 open-craft/openedx-learning#20

Closed



		def create_hash_digest(data_bytes):
		def create_hash_digest(data_bytes: bytes) -> str:

Conversation

bradenmacdonald commented Aug 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openedx-webhooks commented Aug 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rpenido left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bradenmacdonald commented Aug 24, 2023

Uh oh!

bradenmacdonald commented Aug 24, 2023

Uh oh!

ormsbee left a comment

Choose a reason for hiding this comment

Uh oh!

openedx-webhooks commented Aug 24, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

bradenmacdonald commented Aug 16, 2023 •

edited

Loading

openedx-webhooks commented Aug 16, 2023 •

edited

Loading

rpenido left a comment •

edited

Loading