Skip to content

fix: incorrect type hints in a few places - #33104

Merged
bradenmacdonald merged 1 commit into
openedx:masterfrom
open-craft:braden/fix-wrong-type-hints
Aug 28, 2023
Merged

fix: incorrect type hints in a few places#33104
bradenmacdonald merged 1 commit into
openedx:masterfrom
open-craft:braden/fix-wrong-type-hints

Conversation

@bradenmacdonald

@bradenmacdonald bradenmacdonald commented Aug 25, 2023

Copy link
Copy Markdown
Contributor

Description

As part of my Core Contributor role, I have been working on rolling out type hint declarations and checking in more parts of the codebase. I am about to "turn on" type hints for opaque-keys, but before I can do so I need to fix some minor type errors that exist in learning_sequences. These weren't detected before because all opaque keys were being type-checked as Any, but once openedx/opaque-keys#259 merges, opaque-keys become distinct types and mypy will catch errors like this.

The errors:

openedx/core/djangoapps/content/learning_sequences/api/processors/schedule.py:131: error: Invalid index type "UsageKey" for "Dict[str, Dict[str, datetime]]"; expected type "str"  [index]
openedx/core/djangoapps/content/learning_sequences/api/processors/schedule.py:132: error: Invalid index type "UsageKey" for "Dict[str, Dict[str, datetime]]"; expected type "str"  [index]
openedx/core/djangoapps/content/learning_sequences/api/processors/schedule.py:145: error: Invalid index type "UsageKey" for "Dict[str, Dict[str, datetime]]"; expected type "str"  [index]
openedx/core/djangoapps/content/learning_sequences/api/processors/schedule.py:160: error: Invalid index type "UsageKey" for "Dict[str, Dict[str, datetime]]"; expected type "str"  [index]
openedx/core/djangoapps/content/learning_sequences/api/processors/enrollment_track_partition_groups.py:42: error: Argument 1 to "get_user_partition_groups" has incompatible type "CourseKey"; expected "str"  [arg-type]

Note that in this case these aren't runtime bugs, just errors with the type annotations.

Supporting information

See openedx/opaque-keys#259 , openedx/opaque-keys#256 , #32591 for more context.

Testing instructions

The CI should be sufficient, but if you really want:

  1. Run mypy for edx-platform without this change. No errors, but that's because opaque key types are not really being checked.
  2. Check out fix: type hints weren't installed properly, add type hints for Django Fields opaque-keys#259 and use pip install -e . to install the newer type hints into your virtual env.
  3. Run mypy for edx-platform again - you see the errors I mentioned.
  4. Check out this branch and run mypy one final time - the errors are gone.

Deadline

None

Other information

This needs to merge before the corresponding opaque-keys PR.

Private-ref: MNG-3826

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

Copy link
Copy Markdown

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.

""" Read-only data model for User Clipboard data (copied OLX) """
content: StagedContentData = field(validator=validators.instance_of(StagedContentData))
source_usage_key: UsageKey = field(validator=validators.instance_of(UsageKey))
source_usage_key: UsageKey = field(validator=validators.instance_of(UsageKey)) # type: ignore[type-abstract]

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The warning here was openedx/core/djangoapps/content_staging/data.py:66: error: Only concrete class can be given where "Type[UsageKey]" is expected [type-abstract]

I don't think it's a valid warning so I've just ignored it.

@Agrendalath Agrendalath left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

👍

  • I tested this: checked that this fixes the errors that appeared after applying the changes from the mentioned edx-opaque-keys PR
  • I read through the code
  • I checked for accessibility issues: n/a
  • Includes documentation: n/a
  • I made sure any change in configuration variables is reflected in the corresponding client's configuration-secure repository: n/a

@bradenmacdonald
bradenmacdonald merged commit a013c08 into openedx:master Aug 28, 2023
@bradenmacdonald
bradenmacdonald deleted the braden/fix-wrong-type-hints branch August 28, 2023 17:34
@openedx-webhooks

Copy link
Copy Markdown

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

@bradenmacdonald

Copy link
Copy Markdown
Contributor Author

Thanks @Agrendalath !

@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

Yagnesh1998 pushed a commit to ManpraXSoftware/edx-platform that referenced this pull request Sep 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

open-source-contribution PR author is not from Axim or 2U

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

4 participants