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 CompositionView #10590

Merged
merged 12 commits into from
Jan 21, 2022
Merged

remove CompositionView #10590

merged 12 commits into from
Jan 21, 2022

Conversation

jfcarmonag
Copy link
Contributor

@jfcarmonag jfcarmonag commented Dec 27, 2021

CompositionView has been deprecated

Proposed changes:

  • Remove CompositionView

Status (please check what you already did):

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

CompositionView has been deprecated
@jfcarmonag jfcarmonag requested a review from a team as a code owner December 27, 2021 17:18
@jfcarmonag jfcarmonag requested review from wochinge and removed request for a team December 27, 2021 17:18
@CLAassistant
Copy link

CLAassistant commented Dec 27, 2021

CLA assistant check
All committers have signed the CLA.

@@ -22,7 +22,7 @@
from rasa.core.lock_store import LockStore, RedisLockStore, InMemoryLockStore
from rasa.utils.endpoints import EndpointConfig, read_endpoint_config
from sanic import Sanic
from sanic.views import CompositionView
from sanic.views import HTTPMethodView
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the contribution! Can you please also adapt the usage in Line 129?

Copy link

Choose a reason for hiding this comment

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

FWIW, HTTPMethodView and CompositionView are not interchangeable.

Copy link

Choose a reason for hiding this comment

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

You probably want to change this:

        if not isinstance(route.handler, CompositionView):
            handlers = [
                (list(route.methods)[0], route.name.replace("rasa.server.", ""))
            ]
        else:
            handlers = [
                (method, find_route(v.__name__, endpoint) or v.__name__)
                for method, v in route.handler.handlers.items()
            ]

to this:

        handlers = [
            (list(route.methods)[0], route.name.replace("rasa.server.", ""))
        ]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Thanks.

CompositionView has been deprecated
@jfcarmonag
Copy link
Contributor Author

It didn't pass the Code Quality test. I don't understand why. Any help?

@tayfun
Copy link
Contributor

tayfun commented Jan 5, 2022

It didn't pass the Code Quality test. I don't understand why. Any help?

@jfcarmonag Are you able to install and run black on rasa/core/utils.py file? See the exact error here: https://github.com/RasaHQ/rasa/runs/4704649427?check_suite_focus=true

Black is basically the code formatter so that we have consistent and uniform code.

@jfcarmonag jfcarmonag changed the title update CompositionView to HTTPMethodView remove CompositionView Jan 10, 2022
@jfcarmonag
Copy link
Contributor Author

There were some problems with Codeclimate that I didn't understand, but I don't see the messages anymore because I tried to merge again. Any help?

@tayfun
Copy link
Contributor

tayfun commented Jan 19, 2022

There were some problems with Codeclimate that I didn't understand, but I don't see the messages anymore because I tried to merge again. Any help?

Neither of the two errors are on your side @jfcarmonag . The docs one I'm asking around if we need to change netlify config and the other error seems to be related to poetry which an upgrade should fix. I'm making some inquiries to get both resolved. Thanks for your patience.

@tayfun tayfun merged commit 4f57c16 into RasaHQ:main Jan 21, 2022
@tayfun
Copy link
Contributor

tayfun commented Jan 21, 2022

This is now merged, thanks for the contribution @jfcarmonag

Doc build failure was not a blocker because it is not required (but anyway fixed) and the Windows test failure that I thought was due to Poetry error fixed itself on a subsequent run.

tayfun pushed a commit that referenced this pull request Jan 27, 2022
* Remove CompositionView

CompositionView has been deprecated

Co-authored-by: Tayfun Sen <[email protected]>
tayfun pushed a commit that referenced this pull request Jan 28, 2022
* Remove CompositionView

CompositionView has been deprecated

Co-authored-by: Tayfun Sen <[email protected]>

Co-authored-by: Juan Felipe Carmona <[email protected]>
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.

5 participants