-
-
Notifications
You must be signed in to change notification settings - Fork 139
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 nested dict support #208
Merged
Merged
Changes from all commits
Commits
Show all changes
20 commits
Select commit
Hold shift + click to select a range
b117e5e
add nested dict support
synodriver a2ebf1c
revert ide's change
synodriver b9bc4a7
resolve conflict
synodriver 35dffcf
make keyword arguments clear
synodriver 8623bbb
add overflow check
synodriver e2c5faa
better isinstance check
synodriver 12b21d0
add limit for circular references data structures
synodriver 642b375
add support for self-ref objects
synodriver 04b1bcd
merge from upstream
synodriver 0684e26
better recursive call
synodriver 56f6dac
clean up annotations
synodriver 3c321ca
better tests
synodriver b21907f
add python list with same value
synodriver 31edf83
use suggested changes from maintainer
synodriver 56d50a6
rename mapped_objs to mapped_tables
synodriver 33020ef
Merge branch 'master' into nested-dict
scoder b241664
Improve docstring.
scoder 7a69ee8
add tests
synodriver 0e07ff5
Merge branch 'temp' into nested-dict
synodriver c3ca11a
remove print in py_to_lua_table
synodriver File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may not have been the best example, just something that I came up with trying to understand your implementation. However, a more deeply nested example would be good, to make sure that, say, a dict of lists of dicts of dicts of lists of lists also works, or a (list) table of (mapping) tables. You're only testing one level of nesting, always starting with a dict. Maybe you can generate a data structure (or a few), map it back and forth, and then compare the result to itself? That would allow testing something larger and deeper.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Accutally there are other tests which are far more deeper, for instance, the
test_table_from_self_ref_obj
, which is a infinite deep structure.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't so much referring to the depth, rather different combinations of nestings. It might make a difference whether you're mapping a list of dicts or a dict of lists. It might make a difference whether you're mapping a list of simple values or a list of lists of lists. The tests should cover different combinations. Thus, generating data structures and validating the mapping generically would be great, because it would allow quickly testing a larger range of possible structures, making sure that we're not missing edge cases, and making sure we're touching all mapping code. We're probably not covering all of it currently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I'll try to cover more tests, like
List[Dict]
, Dict[str, Dict], List[List]
, Dict[str, List]```There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about something like this:
I hope it's mostly clear, the idea is to generate 0-2 level data structures of all sorts of nesting combinations, and then validate that the result looks the same in Lua as in Python. The validation would probably involve a couple of type checks and back-mappings Lua->Python, but shouldn't be too complex. The above loops generate somewhat redundant results, AFAICT, but not too bad. Maybe you can come up with something smarter even.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Opps... I found a bug, and it can be reproduced in master branch, just use
lua.table_from(1)
, in this waypy_to_lua_table
got a(1, )
, and as it iterate over the tuple, thePyLongObject
is passed tobut
int
is not iterable. Should fix it somehow so that I can continus with my pr.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The structures generated by this function should also be applied to master branch I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By the way, there might be some gc problem with cpython itself, in the loop test there are some lua tables can't match the corresponding pyobject, but when I test them in a individual test, the problem just gone.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My test code proposal was buggy. Here's a revised version that does not generate single elements:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added this test to the master branch and merged it into this PR. Since the "validation" in the master branch is really rudimentary, it hopefully won't fail here, but can now be adapted to validate the transitive translation.