add handling of unknown types to JSON stream#398
add handling of unknown types to JSON stream#398peendebak wants to merge 5 commits intomicrosoft:masterfrom
Conversation
qcodes/utils/helpers.py
Outdated
| # we cannot convert the object to JSON, just take a string | ||
| s = str(obj) | ||
| finally: | ||
| return s |
There was a problem hiding this comment.
You've mixed control patterns... either use:
try:
return super(...)
except TypeError:
return str(...)
or:
try:
s = super(...)
except TypeError:
s = str(...)
# shouldn't go in a finally because you're avoiding the error
# ie if there were any *other* error, then the finally itself would
# error out because s would not be defined!
return s
alexcjohnson
left a comment
alexcjohnson
left a comment
There was a problem hiding this comment.
I'm OK with this (after addressing my comment), does seem like it's better not to fail in this case, though I'd still much prefer that people convert their data structures to JSON when they enter them into the metadata, rather than asking our encoder to do it - that way metadata is consistent when first acquired and when read back in from disk.
|
@alexcjohnson Added the fix as suggested. |
|
Removing the Incidentally, we don't seem to have any tests for |
alexcjohnson
left a comment
alexcjohnson
left a comment
There was a problem hiding this comment.
Remaining comments are nonblocking.
|
@alexcjohnson @giulioungaretti Added a basic test |
|
@giulioungaretti @alexcjohnson Even tough the 💃 is there, I cannot merge myself any more |
giulioungaretti
left a comment
giulioungaretti
left a comment
There was a problem hiding this comment.
@eendebakpt yes, only core can merge.
Also auto pep8 introduced a lot of wrong empty lines.
In helpers.py L18 and 22, 262 and in basically every function in validators.py
|
@eendebakpt thanks for the PR, will prettyify the tests and merge! |
4 participants
@alexcjohnson @giulioungaretti