Uh oh!
There was an error while loading. Please reload this page.
Fix builtins - #137
Conversation
| # Method | ||
| def getMethods(self, ids=None, codes=None, type=None): | ||
| def getMethods(self, ids=None, codes=None, medtype=None, **kwargs): |
There was a problem hiding this comment.
medtype seems like a misspelling, or else med doesn't seem like a good shorthand for "method". How about mettype or even better, full blown methodtype?
There was a problem hiding this comment.
I'll change to methodtype. This is the most clear.
| a = Annotations | ||
| if type: | ||
| if type == 'action': | ||
| if 'type' in kwargs: |
There was a problem hiding this comment.
Should there be a warning if any other kwarg is passed, besides type? ie, an unhandled/unrecognized kwarg name.
Not just in this function, but everywhere you use this scheme.
There was a problem hiding this comment.
I'll see how to best handle unrecognized kwarg
emiliom
commented
Jan 5, 2018
Very nice scheme!! I really like it, specially how it creates a smooth deprecation path. I made a suggestion or two. I'll hold off on merging until you've had a chance to follow up. |
emiliom
left a comment
There was a problem hiding this comment.
Looks good. I'll merge in a bit.
emiliom
commented
Jan 5, 2018
I'll merge when the CI is completed. |
lsetiawan
commented
Jan 5, 2018
Thanks @emiliom! 👍 |
Overview
This PR addresses #115. I simply put warnings to avoid breaking the API. All older code that uses the builtin arguments should still work.