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

feat/Sanitise resource names to ensure they are valid Python identifiers #200

Closed
wants to merge 1 commit into from

Conversation

jakul
Copy link

@jakul jakul commented Oct 12, 2017

Clean resource names to ensure they:

  • can be used as valid Python identifiers, and
  • match the naming style guide for the operation sanitisation

This is a breaking change, becuase resources which are currently called My Tag will now be called My_Tag. For users of bravado it will mean workarounds like getattr(client, 'My Tag').api_method().result() will need to be replaced with client.My_Tag.api_method().result(). I'm not sure what the impact of that change will be on any other libraries which depend on bravado-core.

Fixes Yelp/bravado#307. I have also proposed a different fix for the same problem in Yelp/bravado#320. I'm not sure if it's better to fix this in bravado-core or bravado (or both).

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 98.001% when pulling 690bcbb on jakul:feat/sanitise-tags-with-spaces into 249c602 on Yelp:master.

@soumyakantiroychowdhury
Copy link

soumyakantiroychowdhury commented Feb 1, 2018

I also faced this issue and bypassed it using getattr(client, 'My Tag').api_method().result(). I am not in favor of your proposed change to rename My Tag to My_Tag internally, if it breaks the existing code after upgrade.

Is it possible to do something like this:
self.client.Resource('My Tag').api_method().result()?

@sjaensch
Copy link
Contributor

sjaensch commented Feb 5, 2018

Two comments:

  1. The bravado SwaggerClient class already has a method _get_resource, you can use it to fetch a resource with special characters. The underscore is just there to avoid name clashes, it's a public method.
  2. This change is not backwards-compatible. It breaks existing code that uses _get_resource or the getattr trick mentioned by @soumyakantiroychowdhury to retrieve the resource.

I've tried a different approach in #243 that also sanitizes param names, let me know what you think.

@sjaensch sjaensch closed this in #243 Feb 6, 2018
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.

Resources created when tags have spaces are inaccessible
4 participants