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

Sqlitedb API implementation #60

Merged
merged 34 commits into from
Aug 23, 2024
Merged

Sqlitedb API implementation #60

merged 34 commits into from
Aug 23, 2024

Conversation

thorbjoernl
Copy link
Collaborator

@thorbjoernl thorbjoernl commented Jul 31, 2024

Change Summary

Provides an initial implementation for storing json blobs in an sqlite database.

image

Related issue number

Checklist

  • Start with a draft-PR
  • The PR title is a good summary of the changes
  • PR is set to AeroTools and a tentative milestone
  • Documentation reflects the changes where applicable
  • Tests for the changes exist where applicable
  • Tests pass locally
  • Tests pass on CI
  • At least 1 reviewer is selected
  • Make PR ready to review

@thorbjoernl thorbjoernl added the enhancement New feature or request label Jul 31, 2024
@thorbjoernl thorbjoernl added this to the m2024-09 milestone Jul 31, 2024
@thorbjoernl thorbjoernl self-assigned this Jul 31, 2024
@thorbjoernl thorbjoernl marked this pull request as ready for review August 9, 2024 12:58
@thorbjoernl thorbjoernl marked this pull request as draft August 9, 2024 12:58
Copy link
Member

@heikoklein heikoklein left a comment

Choose a reason for hiding this comment

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

Congratulations, having two independent implementations is an important step for aerovaldb.

With this step you also refactored much of jsondb code and testing, which was expected, but is also a bit overwhelming when reading the PR. I would have prefered several smaller PRs, e.g. about the refactoring of jsondb, changes introduced in testing and then only sqlitedb. Now, it is impossilbe to see for me where a change is coming from and where it might eventually break functionality. But this just as a note for the future.

What is completely missing is the documention on 'How to implement add an additional aerovaldb implementation', similar to https://pyaro.readthedocs.io/en/latest/how-to-add-new-reader.html . With the addition of sqlitedb you made a lot of decisions about that, and that should be documented. Since the documentation isn't strictly necessary for this PR, I approve.

src/aerovaldb/utils/string_mapper/mapper.py Show resolved Hide resolved
src/aerovaldb/jsondb/jsonfiledb.py Outdated Show resolved Hide resolved
@thorbjoernl
Copy link
Collaborator Author

Thanks for reviewing :) I'll try to keep changes smaller and more manageable in the future.

I've created an issue for the documentation here for future reference: #80

@thorbjoernl thorbjoernl merged commit b44bce0 into main Aug 23, 2024
4 checks passed
@thorbjoernl thorbjoernl deleted the sqlitedb branch August 23, 2024 10:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AerovalDB should support more than one db-format Sqlite API
2 participants