Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor!: split AvroAttrsBridge into distinct methods #60

Merged
merged 18 commits into from
Jun 13, 2022

Conversation

rgraber
Copy link
Contributor

@rgraber rgraber commented May 27, 2022

Description: Split the AvroAttrsBridge into several different modules/classes to separate the schema generation, serialization, and deserialization phases.

JIRA: ARCHBOM-2102
GH Issue: #65

Testing instructions:
Usage described in How-to

Reviewers:

  • tag reviewer
  • tag reviewer

Merge checklist:

  • All reviewers approved
  • CI build is green
  • Version bumped
  • Changelog record added
  • Documentation updated (not only docstrings)
  • Commits are squashed

Post merge:

  • Create a tag
  • Check new version is pushed to PyPI after tag-triggered build is
    finished.
  • Delete working branch (if not needed anymore)

@rgraber rgraber marked this pull request as draft May 27, 2022 18:56
@rgraber rgraber force-pushed the rsgraber/ARCHBOM-2102-refactor-bridge branch 2 times, most recently from bc6d37d to 3d19827 Compare June 6, 2022 17:20
@rgraber rgraber marked this pull request as ready for review June 6, 2022 17:31
Copy link
Contributor

@robrap robrap left a comment

Choose a reason for hiding this comment

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

Nice. I like that the different modules keep a clear distinction between schema, serialization, and deserialization code.

docs/how_tos/avro_module.rst Outdated Show resolved Hide resolved
docs/how_tos/avro_module.rst Outdated Show resolved Hide resolved
docs/how_tos/avro_module.rst Outdated Show resolved Hide resolved
openedx_events/event_bus/avro/deserializer.py Outdated Show resolved Hide resolved
openedx_events/event_bus/avro/deserializer.py Outdated Show resolved Hide resolved
openedx_events/event_bus/avro/schema.py Outdated Show resolved Hide resolved
openedx_events/event_bus/avro/schema.py Outdated Show resolved Hide resolved
openedx_events/event_bus/avro/serializer.py Outdated Show resolved Hide resolved
openedx_events/event_bus/avro/tests/test_schema.py Outdated Show resolved Hide resolved
openedx_events/event_bus/avro/tests/test_utilities.py Outdated Show resolved Hide resolved
@rgraber rgraber force-pushed the rsgraber/ARCHBOM-2102-refactor-bridge branch from 3186715 to 72c0fef Compare June 7, 2022 16:45
Copy link
Contributor

@robrap robrap left a comment

Choose a reason for hiding this comment

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

Thanks Becca. I want to help wrap up this PR quickly, so if anything needs discussion, maybe we can handle that synchronously to land this more easily?

I added lots of comments, but many are suggested changes. If you like the change, I highly recommend you just click the "Add suggestion to batch" button to make your life easier, and then add a commit with all the changes.

Some of the test recommendation updates I made aren't really blockers. If they are quick, please update. Otherwise, you can either note that you are skipping and/or we could add a TODO for potential future clean-up.

Thank you!

CHANGELOG.rst Outdated Show resolved Hide resolved
openedx_events/event_bus/avro/schema.py Outdated Show resolved Hide resolved
docs/decisions/0005-external-event-schema-format.rst Outdated Show resolved Hide resolved
docs/decisions/0005-external-event-schema-format.rst Outdated Show resolved Hide resolved
openedx_events/event_bus/avro/tests/test_deserializer.py Outdated Show resolved Hide resolved
openedx_events/event_bus/avro/tests/test_deserializer.py Outdated Show resolved Hide resolved
openedx_events/event_bus/avro/tests/test_serializer.py Outdated Show resolved Hide resolved
openedx_events/event_bus/avro/tests/test_serializer.py Outdated Show resolved Hide resolved
docs/decisions/0005-external-event-schema-format.rst Outdated Show resolved Hide resolved
@robrap robrap added the event-bus Work related to the Event Bus. label Jun 8, 2022
@rgraber rgraber merged commit b42b13a into main Jun 13, 2022
@rgraber rgraber deleted the rsgraber/ARCHBOM-2102-refactor-bridge branch June 13, 2022 11:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
event-bus Work related to the Event Bus.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants