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

@SergioSim SergioSim self-assigned this Nov 15, 2023
@SergioSim SergioSim force-pushed the unify-http-backends branch 2 times, most recently from da71183 to aad886a Compare November 16, 2023 15:25
@SergioSim SergioSim marked this pull request as ready for review November 16, 2023 16:33
@SergioSim SergioSim force-pushed the unify-http-backends branch 2 times, most recently from 68875b0 to 7fe00fb Compare November 20, 2023 18:04
@wilbrdt wilbrdt added this to the 4.0 milestone Nov 27, 2023
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 SergioSim force-pushed the unify-http-backends branch from 5b89b09 to 5bbd28b Compare December 5, 2023 14:30
@SergioSim SergioSim force-pushed the add-greedy-option branch 2 times, most recently from b550933 to 6956c70 Compare December 8, 2023 08:33
@SergioSim SergioSim force-pushed the unify-http-backends branch from d342c4c to 5fe4fe3 Compare December 8, 2023 08:57
@SergioSim SergioSim force-pushed the unify-http-backends branch from 5fe4fe3 to cf3899d Compare December 9, 2023 01:03
@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