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

🏗️(backends) migrate LRSHTTPBackend to LRSDataBackend #524

Merged
merged 1 commit into from
Dec 12, 2023

Conversation

SergioSim
Copy link
Collaborator

Purpose

The interface of the http backends are now very similar to the data backends.
We want to limit the number of backend interfaces.

Proposal

  • move the LRSHTTPBackend and AsyncLRSHTTPBackend to the data backend package.
  • update the cli write command to align more with the data backend interface.
  • drop inheritance between AsyncLRSDataBackend and LRSDataBackend

Copy link
Contributor

@wilbrdt wilbrdt left a comment

Choose a reason for hiding this comment

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

Alt text
We should also update the documentation for the LRS HTTP backends (even if it's not mature yet, we reference it here https://openfun.github.io/ralph/master/backends/#learning_record_store_lrs_-_http_backend_interface)

.env.dist Outdated Show resolved Hide resolved
src/ralph/backends/data/async_lrs.py Outdated Show resolved Hide resolved
src/ralph/backends/data/async_lrs.py Outdated Show resolved Hide resolved
Copy link
Contributor

@Leobouloc Leobouloc left a comment

Choose a reason for hiding this comment

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

Nice tidying. Looks good to me :)

src/ralph/backends/data/async_lrs.py Outdated Show resolved Hide resolved
tests/fixtures/backends.py Outdated Show resolved Hide resolved
@SergioSim
Copy link
Collaborator Author

SergioSim commented Dec 11, 2023

We should also update the documentation for the LRS HTTP backends (even if it's not mature yet, we reference it here https://openfun.github.io/ralph/master/backends/#learning_record_store_lrs_-_http_backend_interface)

Good point) Thanks) Moved the documentation update related to the LRS data backend made in the WebSocket PR here)

@SergioSim SergioSim force-pushed the add-greedy-option branch 2 times, most recently from 8332dc7 to a316f0d Compare December 12, 2023 04:57
Base automatically changed from add-greedy-option to master December 12, 2023 05:20
The interface of the http backends are now very similar
to the data backends.
Thus, to limit redundant interfaces we opt to move the
LRSHTTPBackend and AsyncLRSHTTPBackend to the data
backend package.
We also update the cli write command to align more with
the data backend interface.
Moreover, the LRSDataBackend no longer inherits from
the AsyncLRSDataBackend to alow it's usage in an async
context.
Finally, the behavior of the sync and async versions of
the lrs data backend are very similar.
Thus, to keep both implementations inline, we tests both
of them in the same file.
@SergioSim SergioSim merged commit e4b84f9 into master Dec 12, 2023
23 of 24 checks passed
@SergioSim SergioSim deleted the unify-http-backends branch December 12, 2023 06:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants