Add type hinting across the package by bmtcril · Pull Request #565 · openedx/openedx-events

bmtcril · 2026-04-17T20:37:00Z

Description

This issue came up, and I had an old set of related changes. This PR should update the code to be mypy compliant and include typing for downstream packages. Several other style issues were fixed along the way, so it is a large set of changes, but should not introduce any new behaviors. If downstream code was using the existing type hinting previously, it may fail linting on these changes and require updates.

Notes:

There is a significant change to user attrs instead of the older attr.s for clearer semantics (simply use type hinting instead of explicitly declaring the type via attr.ib(...) in most cases
There is significant use of Any in this PR due to several places in the code that take wildly varied inputs. This can be tightened up, but I wanted to get this first pass up for review before spending any more time on it
Signals cannot currently be annotated as it breaks edx-lint's static annotation cherker, this PR will unblock that work (I have it stashed). Sadly this is probably the most valuable part for @bradenmacdonald
Commits are made in hopefully reasonable chunks with little-to-no overlap, so it may be easier to review commit-by-commit.

Testing instructions

I'll be adding PRs to the event bus repos and platform to see what the downstream impact is shortly, this will stay in draft until then.

Merge Checklist:

Post Merge:

Create a tag
Create a release on GitHub
Check new version is pushed to PyPI after tag-triggered build is
finished.
Delete working branch (if not needed anymore)
Upgrade the package in the Open edX platform requirements (if applicable)

Claude Sonnet 4.6 was used to modernize changes from an old PR and break them up into smaller commits.

bmtcril · 2026-04-21T17:53:53Z

Kafka test PR is passing (though it was way behind and broken so that PR has a number of other fixes): openedx/event-bus-kafka#344

bmtcril · 2026-04-21T18:14:17Z

Redis bus PR: openedx/event-bus-redis#228

bradenmacdonald

Wow, awesome work - thanks! ✅ LGTM but I'm not a committer on this repo :)

bradenmacdonald · 2026-04-22T20:53:54Z

+[mypy-opaque_keys.*]
+ignore_missing_imports = True


opaque-keys has type info now, so we shouldn't need this.

bradenmacdonald · 2026-04-22T21:00:32Z

+        default=None,
+        converter=_default_id,


Why this converter pattern instead of

Suggested change

default=None,

converter=_default_id,

factory=uuid1,

or

Suggested change

default=None,

converter=_default_id,

default=attrs.Factory(uuid1),

? I think using default or factory like this is simpler, and you don't need to implement the _default_x functions yourself.

The default + converter lets us handle None better, where the factory pattern would ignore any place where we explicitly pass None (tooling.py:88) vs just omitting the param. The converter runs before the validator so it produces an actual value. I wasn't sure if we have other places where we might pass in explicit None but this seemed safer.

bmtcril · 2026-07-13T18:16:14Z

@openedx/committers-openedx-events can folks offer opinions on whether this is a good direction? I can rebase and wrap it up if so.

Adds a new public signal, ``REGISTRATION_DEMOGRAPHICS_CAPTURED``, fired by LMS once a learner has successfully registered and any optional demographic fields collected by an authn-form plugin have been validated. Why a new event rather than extending ``STUDENT_REGISTRATION_COMPLETED``? * ``STUDENT_REGISTRATION_COMPLETED`` carries only ``UserData`` today. Adding optional demographic fields to it would force every existing receiver to deal with the new attrs, and would couple a generic registration event to one specific use case (demographics collection). * A dedicated event lets deployments that don't collect demographics ignore it entirely, and lets deployments that do route it onto the event bus (Kafka/Redis) without touching the higher-volume registration-completed stream. * Versioning the signal independently (``...captured.v1``) means future changes to demographic field shape don't require bumping ``STUDENT_REGISTRATION_COMPLETED``. The payload is intentionally minimal: a ``UserData`` reference plus the two free-form string fields the plugin slot collects (``pronouns``, ``department``). Fields default to empty strings so receivers don't have to special-case "demographics not collected on this deployment". Reference: see the OEX 2026 workshop "Leveraging Open edX Extension Points" running example — this event is the one its registration demographics plugin subscribes to.

navinkarkera

@bmtcril Looks good. Thanks!

Some additional changes were needed here to work around problems inherited from the standard library. - Changing isinstance(obj, colletions.abc.Callable) to callable(obj) - Changing obj back to object, which is what upstream calls it, in spite of that being a redefine

This includes a major refactor from `attr.s` to `attrs` and moves away from a somewhat improper use of default_if_none to explicit functions that can be type checked (vs just taking a generic lambda).

Note: There is an interim state in enterprise for optional types, that needs a fix in Avro handling when we get there. These are identified by:

Note: This also includes several fields that will need to be updated when the Avro later is done, these are identified by: # type: ignore[assignment]

This includes the refactor in schema.py to better handle the None-optional types (see _unwrap_optional). Downstream data files are updated to match.

Note: A lot of these test functions take a large quantity of different input types, so there is a lot of Any usage here. Some of these should probably be broken up, but I'm trying to keep these changes scoped.

bmtcril · 2026-07-21T14:21:23Z

This is the openedx-platform PR that tests these changes. There were a very few places that needed updating:

openedx/openedx-platform#38892

bmtcril · 2026-07-21T14:41:34Z

@mariajgrimaldi @felipemontoya - last check here, I've got a thumb but wanted to make sure you have a chance to take a look before I merge it.

@robrap - this is just a heads up in case you wanted to look for potential bus issues

robrap · 2026-07-21T16:12:09Z

@robrap - this is just a heads up in case you wanted to look for potential bus issues

@bmtcril: Thanks for the heads up. I don't plan on reviewing in detail, and hope the tests were enough to keep things backward-compatible. Good luck.

bradenmacdonald reviewed Apr 22, 2026

View reviewed changes

navinkarkera approved these changes Jul 14, 2026

View reviewed changes

Comment thread openedx_events/event_bus/avro/custom_serializers.py

Comment thread openedx_events/content_authoring/data.py

Comment thread openedx_events/learning/data.py

Tycho Hob added 14 commits July 14, 2026 12:55

style: Add type hinting infrastructure, mypy

984ebfe

style: Type hints for exceptions.py

80b6f6b

style: Annotate data.py

7105eda

This includes a major refactor from `attr.s` to `attrs` and moves away from a somewhat improper use of default_if_none to explicit functions that can be type checked (vs just taking a generic lambda).

style: Add annotations for analytics and enterprise

76a35be

Note: There is an interim state in enterprise for optional types, that needs a fix in Avro handling when we get there. These are identified by:

style: Add annotations for content_authoring

2eebc80

style: Add annotations for learning

7df691b

Note: This also includes several fields that will need to be updated when the Avro later is done, these are identified by: # type: ignore[assignment]

style: Update annotations in Avro

2131cd0

This includes the refactor in schema.py to better handle the None-optional types (see _unwrap_optional). Downstream data files are updated to match.

style: Annotate tooling.py

7582dcc

style: Fix annotation for SIMPLE_PYTHON_TYPE_TO_AVRO_MAPPING

b6afb1b

style: Update annotations for commands, apps, testing.py

8c92eec

style: Update annotations for tests

175934d

Note: A lot of these test functions take a large quantity of different input types, so there is a lot of Any usage here. Some of these should probably be broken up, but I'm trying to keep these changes scoped.

fix: Remove accidentally committed test data

f0843b5

style: Clean up unnecessary lint ignore

c0d0f25

bmtcril force-pushed the bmtcril/type_hints branch from 1103790 to c0d0f25 Compare July 15, 2026 13:41

bmtcril marked this pull request as ready for review July 15, 2026 13:41

	default=None,
	converter=_default_id,
	default=attrs.Factory(uuid1),

		[mypy-opaque_keys.*]
		ignore_missing_imports = True

Uh oh!

Conversation

bmtcril commented Apr 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Testing instructions

Uh oh!

bmtcril commented Apr 21, 2026

Uh oh!

bmtcril commented Apr 21, 2026

Uh oh!

bradenmacdonald left a comment

Choose a reason for hiding this comment

Uh oh!

bradenmacdonald Apr 22, 2026

Choose a reason for hiding this comment

Uh oh!

bradenmacdonald Apr 22, 2026

Choose a reason for hiding this comment

Uh oh!

bmtcril Jul 14, 2026

Choose a reason for hiding this comment

Uh oh!

bmtcril commented Jul 13, 2026

Uh oh!

navinkarkera left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bmtcril commented Jul 21, 2026

Uh oh!

bmtcril commented Jul 21, 2026

Uh oh!

robrap commented Jul 21, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

bmtcril commented Apr 17, 2026 •

edited

Loading