Unify the code to recursively walk structures and make compatible with Python 3. #93
Conversation
albu-diku
commented
Jul 27, 2024
albu-diku
commented
- support tuples
- string conversion
- define the walking internals only once
albu-diku
force-pushed
the
fix/force-recursive-py3
branch
from
45d89b8 to
7f148f1
Compare
albu-diku
force-pushed
the
fix/force-recursive-py3
branch
from
c64af9a to
ca47e01
Compare
There was a problem hiding this comment.
While there's definitely some general progress and valuable unit tests here I think it needs another round of adjustments to be correct and 'complete'.
Do you feel like giving it another go or should I perhaps wrap up my comments+suggestions in another PR for you to inspect?
Given it another go after discussion in-person. What we arrived at is treating the data structure walker more generically and simply passing it the function to use for evaluating whether it encountered a primitive - this actually fits much better with what it already did with Of futher note is that this PR is more like a deduplication of the walking logic at this point, mostly after noticing that it had ended up unintentionally doing two separate things - fixing tuples and refactoring the logic. The former is better done as the minimal thing required in migwsgi coverage (which is what this spun out from) and in doing so makes the two PRs independent. |
| @@ -488,12 +490,28 @@ def verify_local_url(configuration, req_url): | |||
| return False | |||
|
|
|||
|
|
|||
| def is_bytes_type(thetype): | |||
| """Return boolean indicating if val is a unicode string. We avoid | |||
There was a problem hiding this comment.
You mean "if thetype corresponds to a byte sequence / string" ... looks like unfinished copy / paste.
Comparing to bytes works but perhaps it would be slightly clearer to compare with type(b"") here for symmetry with the is_unicode_type helper. One could even refactor them into one, then, but that's another matter :-)
|
|
||
|
|
||
| class MigSharedBase__force_default_fs_coding_rec(MigTestCase): | ||
| """Unit tests of mig.shared.base force_default_fs_coding_rec()""" |
There was a problem hiding this comment.
This version of the test suite looks a lot better in terms of correctness and coverage, as we discussed previously.
I still think we should either emphasize in the docstring or a comment that we explicitly test output contents to be byte values (b"XYZ") because we already know that the force_default_fs_coding* functions always must return bytes. Without that information one should really compare the recursive output parts against the plain force_default_fs_coding(val) in one test series and perhaps add another test series to check that the non-recursive version returns bytes to complete the chain.
|
|
||
| self.assertEqual(output, b'foobar') | ||
|
|
||
| def test_encode_within_a_dict(self): |
There was a problem hiding this comment.
I might want to duplicate the bytes/unicode permutations from the tuple tests below for these other data types for the sake of completeness if nothing else.
|
Thanks for the latest assert changes. There are some unresolved bits above which I'm slightly uncertain about. So please let me know when you want me to review this again. |
* support tuples * string conversion * define the walking internals only once
albu-diku
force-pushed
the
fix/force-recursive-py3
branch
from
a34927d to
a40a095
Compare
albu-diku
changed the title
