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

Change decorator returned by request_handler to return the handler function #110

Merged
merged 6 commits into from
Aug 19, 2019

Conversation

Exilit
Copy link
Contributor

@Exilit Exilit commented Aug 8, 2019

This allows to call the original function (e.g. to put it under test).
Otherwise the original function name gets bound to NoneType and is not
accesible anymore. (see https://stackoverflow.com/q/57349825/2980895)

Description

This pull request only adds a return handle_func to the wrapper function returned by SkillBuilder.request_handler(). The wrapper returned by this function is a decorator and therefore should return a function by convention.

Motivation and Context

The mentioned wrapper function does currently not return anything. Because of the logic of a decorator, the returned value of a decorator is bound to the original function name (see https://gist.github.com/Zearin/2f40b7b9cfc51132851a). Since the decorator does not return any value the original name is bound to NoneType.

As a result the function is not accessible anymore after decoration. This is not a big deal in general, as you should not need to call the function manually in most cases, but there are two reasons to change that:

  1. To follow best practices.
  2. To put the handler functions under test.

Currently it is not possible to unit test the handler functions, which results in lower code quality and also a more difficult development cycle.

Testing

The primary goal was to make the decorated handler functions testable. The success of the following test case verified the change in the first place:

from skill import some_handler
from tests import new_request

# tests.py

def test_intent():
    request = new_request('LaunchRequest', {'SlotName': 'slot value'})
    response = some_handler(request)

    expected_response = "abc"
    assert response == expected_response
# skill/__init__.py

from ask_sdk_core.skill_builder import SkillBuilder
from ask_sdk_core.utils import is_request_type

sb = SkillBuilder()

@sb.request_handler(can_handle_func=is_request_type("LaunchRequest"))
def some_handler(handler_input):
    return "abc"

To make sure, that this change does not break anything else, I ran the local unittests of the SDK, which finished successfully.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • My code follows the code style of this project
  • My change requires a change to the documentation
  • I have updated the documentation accordingly
  • I have read the README document
  • I have added tests to cover my changes
  • I have extended existing tests to cover my changes
  • All new and existing tests passed

License

  • I confirm that this pull request can be released under the Apache 2 license

…nction

This allows to call the original function (e.g. to put it under test).
Otherwise the original function name gets bound to NoneType and is not
accesible anymore. (see https://stackoverflow.com/q/57349825/2980895)
Copy link
Contributor

@nikhilym nikhilym left a comment

Choose a reason for hiding this comment

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

This change is good. However, since we provide decorators across request handler, exception handler, request interceptor and response interceptor, it would be better to make the same changes across all these functions.

@nikhilym nikhilym added bug Something isn't working response-needed labels Aug 13, 2019
@Exilit
Copy link
Contributor Author

Exilit commented Aug 14, 2019

You are absolutely right, i did not think about the other decorators before.
I will have a look into the other decorators and update this PR.

@Exilit
Copy link
Contributor Author

Exilit commented Aug 15, 2019

@nikhilym I added the return statements to the other decorators aswell and extended the tests to make sure the returned function is correct.

Copy link
Contributor

@nikhilym nikhilym left a comment

Choose a reason for hiding this comment

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

:shipit:

@nikhilym nikhilym merged commit e7438eb into alexa:master Aug 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants