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

TagsField is case-sensitive #130

Open
lllama opened this issue Jul 26, 2021 · 4 comments
Open

TagsField is case-sensitive #130

lllama opened this issue Jul 26, 2021 · 4 comments
Assignees

Comments

@lllama
Copy link

lllama commented Jul 26, 2021

API Gateway domain names (and other API Gateway resources tbh) have their keys set to all lowercase letters. (an example can be seen in the response here: https://boto3.amazonaws.com/v1/documentation/api/latest/reference/services/apigateway.html#APIGateway.Client.get_domain_names)

Because TagsField defaults to optional=True, the lowercase key will be missed and the tags will be empty.

@jbmchuck
Copy link
Contributor

Thanks for the report! I'm hesitant to modify the existing TagField class but I'd definitely be open to adding a similar Field class which also produces a LinkCollection of TagLinks, in this case something like LowerCaseTagDictField which would expect a key tags containing a dict (rather than list of dict with Key, Value keys). Do you see any issues with that?

@lllama
Copy link
Author

lllama commented Jul 26, 2021

That seems fine. Explicit being better than implicit and all that. My only thought is whether AWS is moving to the lowercase example for future APIs. In which case a TagsV2Field might make more sense. But that would just be a guess as to what AWS are planning.

Happy with the proposed solution.

@jbmchuck jbmchuck self-assigned this Jul 26, 2021
@lllama
Copy link
Author

lllama commented Jul 27, 2021

A further thought after sleeping in it: at the moment, if the lowercase tags key is present, then the existing TagsField will silently fail to include the tags. It feels like this could be tricky to debug. Maybe check for the lowercase and print a warning?

@jbmchuck
Copy link
Contributor

Sounds good. Due to other obligations and vacation it's fairly unlikely I'll be able to work on this until August 9th at the earliest but feel free to submit a PR if you end up implementing it.

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

No branches or pull requests

2 participants