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) use common utilities among backends #510

Merged
merged 1 commit into from
Nov 24, 2023

Conversation

SergioSim
Copy link
Collaborator

@SergioSim SergioSim commented Nov 13, 2023

Purpose

During data backend unification work, some shared utilities were developed to factorize duplication:
parse_dict_to_bytes, read_raw and iter_by_batch.
However, some backends still used their own implementation of these utilities, leading to some minor behavioral differences among backends.

Proposal

  • update backends to use common utilities.
  • make parse_bytes_to_dict more generic (parse_to_dict) to improve re-usability (see usage in ClickHouseDataBackend).

Note: This PR depends on #501

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.

This cleanup is very welcome !

Reviewed up to mongo backend.

Two of my comments don't apply to the code you changed but could be within the scope of this PR.

src/ralph/backends/data/async_mongo.py Outdated Show resolved Hide resolved
src/ralph/backends/data/clickhouse.py Outdated Show resolved Hide resolved
src/ralph/backends/data/es.py Outdated Show resolved Hide resolved
src/ralph/backends/data/fs.py Show resolved Hide resolved
src/ralph/backends/data/fs.py Show resolved Hide resolved
src/ralph/backends/data/fs.py 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.

(completing my previous review) This tidying is much appreciated !

src/ralph/backends/data/swift.py Show resolved Hide resolved
src/ralph/backends/data/swift.py Show resolved Hide resolved
tests/backends/data/test_fs.py Outdated Show resolved Hide resolved
tests/backends/data/test_ldp.py Outdated Show resolved Hide resolved
tests/backends/data/test_s3.py Show resolved Hide resolved
@SergioSim SergioSim force-pushed the generic-data-backends branch from 2257aba to 02aed1d Compare November 16, 2023 17:00
@SergioSim SergioSim force-pushed the unify-backend-utilities branch from 09baf10 to 71e038d Compare November 16, 2023 17:04
@SergioSim SergioSim force-pushed the generic-data-backends branch from 02aed1d to 525f9c1 Compare November 17, 2023 14:06
@SergioSim SergioSim force-pushed the unify-backend-utilities branch from 1fc4872 to d5c7d3c Compare November 17, 2023 14:08
Base automatically changed from generic-data-backends to master November 22, 2023 15:25
@SergioSim SergioSim force-pushed the unify-backend-utilities branch from c7b75e3 to d426d70 Compare November 22, 2023 15:27
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
Have some nitpicking suggestions, otherwise looks good to me!

src/ralph/backends/data/async_es.py Show resolved Hide resolved
src/ralph/utils.py Outdated Show resolved Hide resolved
tests/backends/data/test_s3.py Show resolved Hide resolved
@wilbrdt wilbrdt added this to the 4.0 milestone Nov 23, 2023
@SergioSim SergioSim force-pushed the unify-backend-utilities branch 5 times, most recently from d40480a to e609c67 Compare November 24, 2023 15:47
src/ralph/utils.py Outdated Show resolved Hide resolved
During data backend unification work some shared utilities were
developed to factorize duplication:
`parse_dict_to_bytes`, `read_raw` and `iter_by_batch`.
However, some backends still used their own implementation of
these utilities leading to some minor behavioral differences
among backends.
Thus, for consistency, we update backends to use common utilities.
@SergioSim SergioSim force-pushed the unify-backend-utilities branch from 67b0601 to 89d588d Compare November 24, 2023 15:55
@SergioSim SergioSim merged commit 55b4d64 into master Nov 24, 2023
21 checks passed
@SergioSim SergioSim deleted the unify-backend-utilities branch November 24, 2023 17:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants