fix for issue 3053. Added check to determine if variable meta is a dict #3054
Conversation
Fixes for #3053. Changed dict check to use isinstance
| @@ -350,7 +350,7 @@ def get_topic_meta_map(self): | |||
| 'SELECT topic_id, metadata ' | |||
| 'FROM {}').format(Identifier(self.meta_table)) | |||
| rows = self.select(query) | |||
| meta_map = {tid: jsonapi.loads(meta) if meta else None for tid, meta in rows} | |||
| meta_map = {tid: meta if meta and isinstance(meta, dict) else jsonapi.loads(meta) if meta else None for tid, meta in rows} | |||
There was a problem hiding this comment.
Just for my understanding, is this nested ternary expression one-liner equivalent to the following for-loop?
meta_map = {}
for tid, meta in rows:
if meta:
if meta and isinstance(meta, dict):
meta_map[tid] = meta
else:
meta_map[tid] = jsonapi.loads(meta)
else:
meta_map[tid] = None
There was a problem hiding this comment.
Just curious. Is obfuscation best practice for this project?
There was a problem hiding this comment.
@awoz No it's not
I think nested if looping this way is harder to read as well. The original meta_map was the right level of complexity, but I did have a hard time following the new code.
@ntenney can you replace the code with something more user friendly.
@ntenney If it helps, feel free to copy and/or modify the for loop in my initial comment above ^^
There was a problem hiding this comment.
I'm not a huge fan of splitting something that takes 1 line into 9. Would this be more palatable?
meta_map = {tid: meta if meta and isinstance(meta,dict) else (json.loads(meta) if meta else None) for tid, meta in rows}
or
meta_map = {tid: meta if (meta and isinstance(meta,dict)) else (json.loads(meta) if meta else None) for tid, meta in rows}
There was a problem hiding this comment.
@ntenney splitting the long line or adding the parenthesis doesn't make it any clearer for me. I do get what the code is doing but I agree that the broken down for loop was much easier to read. I might have voted for the compact format if there was just a single if inside but the second if inside for loop inside the {} really pushes it over the edge for me
There was a problem hiding this comment.
It is my general sense that all source code should be written "for other" developers who may not be familiar with your code. For them to be successful in their roles, source code hints in the form of comments or clean format are essential. This principle allows for an efficient coding, debug and review cycle, with a timely delivery of solutions. I'm pretty sure that understanding and debugging / single stepping a one-liner is not very palatable.
There was a problem hiding this comment.
It is my general sense that all source code should be written "for other" developers who may not be familiar with your code. For them to be successful in their roles, source code hints in the form of comments or clean format are essential. This principle allows for an efficient coding, debug and review cycle, with a timely delivery of solutions. I'm pretty sure that understanding and debugging / single stepping a one-liner is not very palatable.
@awoz Agree. That's why I made the initial comment because I had a hard time understanding and visualizing the control flow of the dictionary comprehension, especially since the comprehension requires one to read it from right to left as opposed to the conventional (i.e. Western) way of reading from left to right.
I drafted up the equivalent for loop to both confirm that the control flow of the dictionary comprehension was the intended flow; and to elicit feedback from others on best practices. Thank you for your feedback.
Make for loop more pretty
|
@ntenney Thank you! |
Description
Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.
Fixes #3053
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
This code has been run against databases containing jsonb type and strings for metadata. In addition, I used this code against a local list that simulated the results coming from the database, and also tested it against a situation that should never arise in the wild (strings mixed with objects in the metadata result column). This was tested using the 8.2 deploy of campus.