ADR for Public Python API Conventions - #186
Conversation
|
@kdmccormick, @bradenmacdonald: Please review the ADR here. |
|
I'm going to hold this ADR open for at least a week, (so until May 6th), to give anyone in the wider community a chance to comment on it. Edit: Advertised here: https://discuss.openedx.org/t/adr-on-python-public-api-conventions-in-learning-core/12872 |
| App ``api`` modules should not import directly from apps outside their package. | ||
| For example, ``openedx_learning.apps.personalization.api`` should import authoring API functions from ``openedx_learning.api.authoring``, **not** directly from something like ``openedx_learning.apps.authoring.components.api``. This will help to limit the impact of refactoring app package internal changes, as well as exposing shortcomings in the existing public APIs. | ||
|
|
||
| Public API modules may implement their own functions. |
There was a problem hiding this comment.
I like this. Not only for convenience but also as a way to address the natural evolution of the API. Do you think this might replace versioning by creating shims and deprecating functions with good documentation and timelines?
There was a problem hiding this comment.
I'm not sure if it will replace versioning altogether, but I definitely plan to use compatibility shims more liberally here than we have in for Django apps historically, and to make deprecations happen at the level of individual functions with deprecation warnings.
felipemontoya
left a comment
felipemontoya
left a comment
There was a problem hiding this comment.
I like this proposal. Left a question, but mostly curious about it. Nothing blocking
bradenmacdonald
left a comment
bradenmacdonald
left a comment
There was a problem hiding this comment.
Sounds good. It would be nice to update the import linter in edx-platform to make sure that people are following this.
@felipemontoya: Thank you for looking at it so quickly! |
|
|
||
| This relies on the individual apps to properly set ``__all__`` to the list of functions that they are willing to publicly support. | ||
|
|
||
| App ``api`` modules within a package of apps still import from each other. |
There was a problem hiding this comment.
Is it worth having a convention of _api to indicate more local APIs that are meant to be used internal to the project but is not reliable outside of the openedx-learning project? It seems cumbersom but also could help reduce the surface area of API that we have to guarantee to parties that build on top of openedx-learning.
There was a problem hiding this comment.
That's an interesting idea. I guess it partly depends on where the maintenance boundaries end up being. This arrangement makes more sense to me if a single group of people is responsible for the broad package of authoring apps, as opposed to multiple teams working on separate groups of apps in authoring (which is... probable...?). I admit that part of my resistance to this idea might just be aesthetic–it feels weird to me that the components app would import from a private module of publishing (openedx_learning.apps.publishing._api).
I'm hoping that between documentation and importlinter rules in edx-platform, it'll be clear that consumers should only import from modules in the openedx_learning.api package. But I'm definitely open to revisiting this if it turns out to be a source of confusion.
|
Updated ADR to record feedback from @bradenmacdonald and @feanil. |
feanil
left a comment
feanil
left a comment
There was a problem hiding this comment.
I'd love to see the import linting rules for this in edx-platform as close to merging this as possible.
I'll include it as part of the refactoring PR necessary on the edx-platform side to switch over. |
4 participants
This proposes refactoring our apps and introducing two major new conventions at the same time:
openedx_learning.corewould now be inopenedx_learning.apps.authoring, setting a pattern for grouping apps together into larger package groups.openedx_learning.api.authoringmodule would be created to aggregate API modules from apps inopenedx_learning.apps.authoring.I originally wrote this up as a part of the major refactoring in #184, but separated out here so that we can discuss it in its own space.