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

chore(weave): refactor objects query into a simple query builder #3223

Open
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

gtarpenning
Copy link
Member

@gtarpenning gtarpenning commented Dec 12, 2024

Description

Straight refactor for readability. Use a query builder to handle adding conditions to the objects query. Considerably less extensive or flexible as the other query builders, but the objects query is also much more complex. Not sure we need as much flexibility here, this pr should be a balance between risky and merely surface level.

Testing

Existing code coverage + unit tests

@circle-job-mirror
Copy link

circle-job-mirror bot commented Dec 12, 2024

@gtarpenning gtarpenning changed the title Griffin/objects db refactor chore(weave): refactor objects query into a simple query builder Dec 13, 2024
@gtarpenning gtarpenning marked this pull request as ready for review December 13, 2024 20:37
@gtarpenning gtarpenning requested a review from a team as a code owner December 13, 2024 20:37
Copy link
Member Author

gtarpenning commented Dec 13, 2024



def test_make_object_id_conditions_part():
assert _make_object_id_conditions_part(None) == ""
Copy link
Collaborator

Choose a reason for hiding this comment

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

seems like a weird function to me - it assumes that it always comes after a non-empty where clause?

Copy link
Member Author

Choose a reason for hiding this comment

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

Its meant to be an internal fn but i see your point. i'll rename and add a bit of logic here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants