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

Add ordered dictionary #1

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

Conversation

Totktonada
Copy link

The module is inspired by Python's collections.OrderedDict. See the description in the module code for details.

Borrowed from tarantool/tarantool#9730

@ochaton ochaton self-assigned this Mar 12, 2024
@ochaton
Copy link
Member

ochaton commented Mar 12, 2024

Thanks for the great patch! I do imagine that someone will need this lib at a very specific time :)

tests are all great along with the high coverage, but github worklows fucked us up with sharing secrets and variables to this PR.

We've discussed offline, that we will extend a little bit explanation for this module (I didn't know what exactly Python's OrderedDict guarantees for users), we will add section to README.md and we will adopt some tests from original python OrderedDict.

The module is inspired by Python's collections.OrderedDict. See the
description in the module code for details.

Borrowed from tarantool/tarantool#9730 as is
with layout changes and a few code style changes (`require('foo')` vs
`require 'foo'`).
@Totktonada
Copy link
Author

Thanks for the warm feedback!

I've moved the description of the module to README. I've expanded it with one more example and I hope that it should be more clear now.

we will adopt some tests from original python OrderedDict.

It is on me, not done yet.

@Totktonada
Copy link
Author

NB: Backported the fix for the ffi.new('void *') keys to tarantool in tarantool/tarantool#10080.

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