Skip to content

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

Merged
mpenkov merged 4 commits into
piskvorky:masterfrom
rdyro:key_encoding
Oct 27, 2022
Merged

adding optional (not enabled by default) ability to encode keys#161
mpenkov merged 4 commits into
piskvorky:masterfrom
rdyro:key_encoding

Conversation

@rdyro

@rdyro rdyro commented Oct 21, 2022

Copy link
Copy Markdown
Contributor

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
Comment thread 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
Copy Markdown
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
Copy Markdown
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

mpenkov commented Oct 23, 2022

Copy link
Copy Markdown
Collaborator

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

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

@rdyro

rdyro commented Oct 23, 2022

Copy link
Copy Markdown
Contributor Author

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

rdyro commented Oct 27, 2022

Copy link
Copy Markdown
Contributor Author

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

mpenkov commented Oct 27, 2022

Copy link
Copy Markdown
Collaborator

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

2 participants