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

adding optional (not enabled by default) ability to encode keys #161

Merged
merged 4 commits into from
Oct 27, 2022

Conversation

rdyro
Copy link
Contributor

@rdyro rdyro commented Oct 21, 2022

This pull requests adds the optional behavior of encoding keys.

The justification: support for hashable, but not str (or int) keys, e.g. tuple as a key.

The changes include:

  • the core class now allows passing encode_key and decode_key method
  • both of these methods are the identity function by default
  • the module now provides two default encode_key and decode_key which are base64 encoding of pickle dump of key
  • the changes do not change the sqlite key type: TEXT
  • the changes are tested, I added a test for a tuple key

Potential problems:

  • the optional key encoding using pickle serialization, so it allows non-hashable types to act as keys

sqlitedict.py Outdated
Comment on lines 102 to 103
autocommit=False, journal_mode="DELETE", encode=encode,
decode=decode, timeout=5, outer_stack=True):
#decode=decode, encode_key=identity, decode_key=identity,
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is this commented line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ups, a mistake. I just pushed a fix.

@mpenkov
Copy link
Collaborator

mpenkov commented Oct 23, 2022

Can you please fix the flake8 errors in the code as well?

https://github.com/RaRe-Technologies/sqlitedict/actions/runs/3303841568/jobs/5456104420

@rdyro
Copy link
Contributor Author

rdyro commented Oct 23, 2022

I just fixed the flake8 errors.

Would documenting this optional key encoding in the README be a good idea? I added a commit for that.

@rdyro
Copy link
Contributor Author

rdyro commented Oct 27, 2022

Could we merge this into master?

This PR adds attempts to add a useful optional feature, while introducing minimum extra complexity and not affecting performance.

@mpenkov mpenkov merged commit 4774476 into piskvorky:master Oct 27, 2022
@mpenkov
Copy link
Collaborator

mpenkov commented Oct 27, 2022

Yes, of course. I've merged your work. Thank you!

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.

None yet

2 participants