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

Fix a bug with unnecessary kwargs unpacking #4802

Merged
merged 3 commits into from
Nov 28, 2019
Merged

Fix a bug with unnecessary kwargs unpacking #4802

merged 3 commits into from
Nov 28, 2019

Conversation

mrkaaa
Copy link
Contributor

@mrkaaa mrkaaa commented Nov 20, 2019

Proposed changes:

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)

@CLAassistant
Copy link

CLAassistant commented Nov 20, 2019

CLA assistant check
All committers have signed the CLA.

@erohmensing
Copy link
Contributor

Thanks for the PR, we'll give it a review as soon as possible.

@erohmensing erohmensing requested a review from wochinge November 20, 2019 18:44
@erohmensing
Copy link
Contributor

@imLew does this have to do with #4458?

@erohmensing erohmensing requested review from imLew and removed request for wochinge November 25, 2019 08:31
@imLew
Copy link
Contributor

imLew commented Nov 25, 2019

@erohmensing it's the same underlying issue (having an argument called kwargs which is not the typical catch-all-additional-keyword-args, ie **kwargs), but it is not the what that issue was referring to.

Looking at this function, I think this PR fixes the superficial issue resulting from using the name kwargs as an argument name. Right now test_core will complain about "unexpected keyword argument" if kwargs is unpacked and is not empty. But this should should addressed in more comprehensive way, either changing the name kwargs or rewritting it to use **kwargs.

Copy link
Contributor

@imLew imLew left a comment

Choose a reason for hiding this comment

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

Thanks for the fix :D, this should indeed not be unpacked.

Copy link
Contributor

@wochinge wochinge left a comment

Choose a reason for hiding this comment

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

Thanks for this fix @mrkaaa Could you please rebase this to go 1.5.x and update the changelog? Thanks 🙏

@tmbo tmbo changed the base branch from master to 1.5.x November 28, 2019 09:24
@tmbo tmbo merged commit 68b18f7 into RasaHQ:1.5.x Nov 28, 2019
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.

6 participants