Skip to content
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 support for tuples in get_attr #3302

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

SamRemis
Copy link
Contributor

The get_attr function is intended to be able to support an input with a path such as [0] and a value of ['foo']and parse out the fist index of the value to get foo as the result.

Without the change in this PR, the such an input will not match the indexing regex, and instead it will reach the else statement and reach the code intended to access dictionary indexes. This will fail with a type error, so this PR updates the regex to properly reach the part of the code that handles indexes without a word string before them.

That code still fails for the same input; it assumes that it is getting the index of a list from a dictionary, so it attempts to call .get() on the base input. This will not work if the input value is not a dictionary, so we need to add the conditional to only call .get() if there is a string before the first open bracket in the part of the path that we are parsing.

Finally, this code can't be reached currently with a list due to how we memoize input parameters to endpoints; all parameter types have to be hash-able. While this function could theoretically get called with a list as input from another context, this is a private interface and it is only internally called from the endpoint provider. Because of this, I've updated the docblock to specify tuple as the input type instead of list.

Copy link
Contributor

@alexgromero alexgromero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM :shipit:

@codecov-commenter
Copy link

codecov-commenter commented Nov 15, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 83.33333% with 1 line in your changes missing coverage. Please review.

Project coverage is 93.18%. Comparing base (b1279af) to head (6fb6221).
Report is 98 commits behind head on develop.

Files with missing lines Patch % Lines
botocore/endpoint_provider.py 83.33% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #3302   +/-   ##
========================================
  Coverage    93.18%   93.18%           
========================================
  Files           66       66           
  Lines        14345    14378   +33     
========================================
+ Hits         13367    13398   +31     
- Misses         978      980    +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants