Skip to content

feat(storage): Implement SqliteRelatedTermsIndex.serialize() method #115

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.

Sign up for GitHub

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

Jump to bottom
Merged
gvanrossum merged 4 commits into from
Dec 23, 2025
Merged

feat(storage): Implement SqliteRelatedTermsIndex.serialize() method #115

gvanrossum merged 4 commits into from
Dec 23, 2025

Conversation

@iamrajiv
Copy link
Contributor

@iamrajiv iamrajiv commented Dec 4, 2025

I noticed the NotImplementedError in the serialize methods while exploring the codebase. I implemented the missing serialize() functionality for:

  • SqliteRelatedTermsFuzzy.serialize()
  • SqliteRelatedTermsIndex.serialize()

gvanrossum
gvanrossum reviewed Dec 21, 2025
View reviewed changes
Copy link
Collaborator

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

Thanks! This looks good except for one nit.

test/test_sqlite_indexes.py
assert fresh_fuzzy is not None
assert await fresh_fuzzy.size() == 3

# Use concrete type to access get_terms (not in interface)
Copy link
Collaborator

@gvanrossum gvanrossum Dec 21, 2025

Choose a reason for hiding this comment

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

I wonder if get_terms() should be added to the interface. (Probably not in this PR unless it's a one-liner -- which it isn't since you'd have to implement it for the memory provider too.)

Copy link
Contributor Author

@iamrajiv iamrajiv Dec 23, 2025

Choose a reason for hiding this comment

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

@gvanrossum I agree get_terms() could be useful in the interface. I'll keep it out of this PR to keep the scope small, but happy to open a follow-up issue/PR to add it to the interface and implement it for both providers if that would be helpful.

@gvanrossum
Copy link
Collaborator

gvanrossum commented Dec 23, 2025

@iamrajiv If you're around could you address my nits? Then we can merge it.

@iamrajiv iamrajiv marked this pull request as ready for review December 23, 2025 11:16
@iamrajiv
Copy link
Contributor Author

iamrajiv commented Dec 23, 2025

Thank you for the review, @gvanrossum. I made the changes you suggested. Could you please review it again?

gvanrossum
gvanrossum approved these changes Dec 23, 2025
View reviewed changes
Copy link
Collaborator

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

Perfect! Merging now.

@gvanrossum gvanrossum merged commit 04b976f into microsoft:main Dec 23, 2025
15 of 27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gvanrossum gvanrossum gvanrossum approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@iamrajiv @gvanrossum