Draft
Conversation
added 12 commits
April 16, 2026 08:41
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.
Contributor
Author
|
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 |
Contributor
Author
|
Redis bus PR: openedx/event-bus-redis#228 |
Contributor
bradenmacdonald
left a comment
bradenmacdonald
left a comment
There was a problem hiding this comment.
Wow, awesome work - thanks! ✅ LGTM but I'm not a committer on this repo :)
Comment on lines
+30
to
+31
| [mypy-opaque_keys.*] | ||
| ignore_missing_imports = True |
Contributor
There was a problem hiding this comment.
opaque-keys has type info now, so we shouldn't need this.
Comment on lines
+129
to
+130
| default=None, | ||
| converter=_default_id, |
Contributor
There was a problem hiding this comment.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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:
attrsinstead of the olderattr.sfor clearer semantics (simply use type hinting instead of explicitly declaring the type viaattr.ib(...)in most casesAnyin 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 itTesting 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:
finished.
Claude Sonnet 4.6 was used to modernize changes from an old PR and break them up into smaller commits.