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.

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

Feat/better dictionaries #135

Closed
wants to merge 2 commits into from
Closed

Feat/better dictionaries #135

wants to merge 2 commits into from

Conversation

juanjoDiaz
Copy link
Collaborator

Here is an alternative to #133

It has several advantages:

  • It encapsulates better the bytestring optimization while keeping the dictionary factory working on strings
  • It does not require a external dependency to pick a temp folder for disk caching
  • It does not require all the additional hashing logic. It simply caches data in a folder specific to simplema's version or wherever the user says
  • It does not use internal functions of simplemma like _load_dictionary_from_disk

I still think that this would be better publish as a separate module and not as part of simplemma.

github-advanced-security[bot]
github-advanced-security bot found potential problems Jun 10, 2024
View reviewed changes
simplemma/strategies/dictionaries/trie_dictionary_factory.py
from functools import lru_cache
from pathlib import Path
import tempfile
from typing import ByteString, Dict, Mapping, Optional

Check notice

Code scanning / CodeQL

Unused import Note

Import of 'ByteString' is not used.
Import of 'Dict' is not used.
@adbar
Copy link
Owner

adbar commented Jun 26, 2024

@juanjoDiaz Now that #133 has been merged do you want to keep working on it or should we close this PR for now?

@juanjoDiaz juanjoDiaz closed this Jul 6, 2024
@Dunedan Dunedan mentioned this pull request Nov 5, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
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
@juanjoDiaz @adbar