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.
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
add nested dict support #208
Changes from 15 commits
b117e5e
a2ebf1c
b9bc4a7
35dffcf
8623bbb
e2c5faa
12b21d0
642b375
04b1bcd
0684e26
56f6dac
3c321ca
b21907f
31edf83
56d50a6
33020ef
b241664
7a69ee8
0e07ff5
c3ca11a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.