openedx / openedx-learning

GNU Affero General Public License v3.0
5 stars 10 forks source link

Enforce quality checks and type hints, improve quality and typing #73

Closed bradenmacdonald closed 1 year ago

bradenmacdonald commented 1 year ago

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:

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

Private-ref: FAL-3476

openedx-webhooks commented 1 year ago

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:

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 commented 1 year ago

@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 commented 1 year ago

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

openedx-webhooks commented 1 year ago

@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.