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

remove kwargs from default channel implementation send methods #4546

Merged
merged 1 commit into from Oct 2, 2019

Conversation

erohmensing
Copy link
Contributor

Proposed changes:

  • remove kwargs from default channel implementation. If users want to support these kwargs and e.g. passing them along to another function (from elements to text with buttons, for example), they can still implement their own send_elements function that does just that.
  • fixes the following multiple values error we have seen with migrating from a send_elements workaround before send_custom_json existed:
  File "c:\anaconda3\envs\rasa_latest\lib\site-packages\rasa\core\channels\chan
nel.py", line 255, in send_elements
    recipient_id, element_msg, element.get("buttons", []), **kwargs
TypeError: send_text_with_buttons() got multiple values for argument 'text'

Status (please check what you already did):

  • made PR ready for code review
  • added some tests for the functionality
  • updated the documentation
  • updated the changelog
  • reformat files using black (please check Readme for instructions)

Copy link
Contributor

@akelad akelad left a comment

Choose a reason for hiding this comment

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

do we even need kwargs in the function definitions then though?

@erohmensing
Copy link
Contributor Author

erohmensing commented Oct 1, 2019

i believe so, because we still want them supported in the subclassed channels e.g. here https://github.com/RasaHQ/rasa/blob/master/rasa/core/channels/telegram.py#L42, if we remove them from the base class then it will yell at unexpected arguments (i think, but maybe that's something idk about python)

@akelad
Copy link
Contributor

akelad commented Oct 1, 2019

ok you can merge

@erohmensing
Copy link
Contributor Author

cool, no rush, i thought i'd sanity check with fede or someone before merging... if i ever get out of meetings lol

@erohmensing erohmensing merged commit 135226b into 1.3.x Oct 2, 2019
@erohmensing erohmensing deleted the no-kwargs branch October 2, 2019 09:30
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.

None yet

2 participants