Update keys() to be able to iterate over a dict by updating the loop … #1492
Conversation
…to use .items(). becuase the structure of carrier was not solely defined, and it was mentioned that it is usually a http request in the spec docs, we gave it the ability to peer into child lists and dicts a well to record the keys from their
|
| for _key, _val in carrier.items(): | ||
|
|
||
| #if we have another dict, lets make a recursive call | ||
| if isinstance(_val, Dict): |
There was a problem hiding this comment.
Consider move the code that append dict to new function
You use twice the same code exactly
|
|
||
| #Check for the Tuple | ||
| if isinstance(list_key, Tuple): | ||
| append_keys(list_key[0]) |
There was a problem hiding this comment.
What does the tuple represent? Do we need the rest of the tuple?
There was a problem hiding this comment.
you know i thought long and hard on this. So the only example of a Tuple ive seen come through is within the header, and the tuple is used to keep devs from modifying header values when they are received. So really every tuple was only 2 keys long and acted as a key value store. So i grab the first one as the key and let the 2nd value drop. for example. the headers come in looking like. Carrier{'headers':[('host','127.0.0.01'),('port','6020')]}. I decided not to spend time on checking if the tuple was longer than 2 because there wouldnt be a defined way for this method to determine what a key was and what a value was over. To be perfectly honest i feel like this is all a little heavy for what this method needs to do, but i dont see any prescribed structure. However I could be convinced that this needs to account and add all tuple values to list just to make sure we get all possible header values.
There was a problem hiding this comment.
Ok sounds good to me, maybe just check if the tuple is not empty so the code will not throw an exception
| returnable = [] | ||
|
|
||
| #internal function that appends keys | ||
| def append_keys(key): |
There was a problem hiding this comment.
I think if you will move the append_keys function outside the keys function will be more clear and readable
There was a problem hiding this comment.
I'm not sure I agree whole heartedly on that. There are many use cases for inner functions, in this case, as a helper function. going this route allowed it to have access to everything within the keys() scope. which was beneficial. And all this does is gives us reusablility within the keys() method. they do not serve a purpose outside of the scope of keys()
|
You need to fix the lint |
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.
The change here is correcting the same as
#1487
and pull request
#1435
as well as partly correcting issues found here
#1478
Fixes # (issue)
1487
1435
Starts addressing 1478
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Test added to pull request, Also tested within GCP using the GCP proprogators
Does This PR Require a Core Repo Change?
Checklist:
See contributing.md for styleguide, changelog guidelines, and more.