Make it possible to create class-based skills #734

gergelypolonkai · 2018-11-13T09:29:08Z

Description

Make it possible to create class-based skills. This can be very useful when the skill needs a helper object, like an SDK for an external API.

Status

UNDER DEVELOPMENT

Type of change

New feature (non-breaking change which adds functionality)
This change requires a documentation update

How Has This Been Tested?

I created a very basic skill class for initial testing, but generic tests are still yet to come.

from opsdroid.matchers import match_regex
from opsdroid.skill import Skill


class TestSkill(Skill):
    @match_regex(r'number [0-9]')
    async def describe_project(self, opsdroid, config, message):
        await message.respond('Yes, that’s a number.')

Checklist:

I have performed a self-review of my own code
I have made corresponding changes to the documentation (if applicable)
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes

jacobtomlinson

Thanks so much for raising this. Looks interesting!

I have a few questions about your design decisions. Could you please respond to each one.

opsdroid/parsers/regex.py

opsdroid/skill.py

jacobtomlinson

I feel like I am understanding more the approach you are taking. However I'm still not happy with it. We should be able to use the same matcher decorators as we do normally.

Let me try to describe the problem you experienced which caused you to fork the API:

When you create a skill class and import it the matchers are run against the unbound methods and update the method attributes with the matcher info.
Then you are creating an instance of the class and creating pointers to the bound methods of the instance.
The existing code then tries to set the config attribute on the bound method which fails because you cannot do that.
Your solution is to store the config in __skill_attrs__ which you are pre-creating in the __init__ method.

Perhaps as an alternative approach you could try this:

Catch the exception raised when trying to set the config on the method and ignore it.
Access the config at skill.__self__.config in the parser.

codecov · 2018-11-13T12:03:28Z

Codecov Report

Merging #734 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #734   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          29     30    +1     
  Lines        1901   1934   +33     
=====================================
+ Hits         1901   1934   +33

Impacted Files	Coverage Δ
opsdroid/web.py	`100% <100%> (ø)`	⬆️
opsdroid/core.py	`100% <100%> (ø)`	⬆️
opsdroid/loader.py	`100% <100%> (ø)`	⬆️
opsdroid/skill/__init__.py	`100% <100%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dc70f4a...d6e5d7f. Read the comment docs.

gergelypolonkai · 2018-11-13T12:05:50Z

I have reworked the whole thing (and learned a bit about Python internals while there). Now it works fine with the original decorators, without modifying the decorators themselves; it was much easier than i initially thought.

jacobtomlinson

This looks even better than I imagined! This is a very nice way to add this functionality.

Could you now add tests and documentation for this.

Also we currently run a setup function for each skill if it exists. However after this change I think we should encourage people to use the class version going forwards and put their setup code in the __init__ method. So could you please add a deprecation warning message when the setup is called telling people that this will be removed in a future version.

jacobtomlinson · 2018-11-13T12:20:00Z

opsdroid/core.py

+                    for name in skill_obj.__dir__():
+                        try:
+                            method = getattr(skill_obj, name)
+                        except Exception:


Can you make this less broad?

Actually, no. Imagine this code:

class MySkill(Skill): @property def erroneous_property(self): return self.sdk_this_skill_uses.get_erroneous_property()

Where get_erroneous_property() can raise any type of exception. Been there, done that 😞

We can either remove the try/except altogether and ask skill writers to look out for this corner case, or leave the except block this broad.

Fair enough! Can you please add a pylint disable with this justification in a commend. See here for an example of this.

gergelypolonkai · 2018-11-13T12:44:08Z

Should the deprecation warning be an actual DeprecationWarning or just a log message like the one for the Api.ai parser?

gergelypolonkai · 2018-11-13T12:49:02Z

Also, should the deprecation message appear if a function based skill is used, even if there is no setup function?

jacobtomlinson · 2018-11-13T13:55:09Z

We should probably be raising deprecation warnings. No we should only show it if the skill uses it.

jacobtomlinson · 2018-11-20T08:33:27Z

@gergelypolonkai I'm really keen to get this merged. Any chance you can pick it up again?

gergelypolonkai · 2018-11-20T09:05:23Z

I’m a bit lost in the tests, but expect to finish soon.

stale · 2018-12-12T15:00:20Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

jacobtomlinson · 2018-12-12T16:24:01Z

Hi @gergelypolonkai. Are you able to pick this up again?

gergelypolonkai · 2018-12-12T17:58:33Z

Most definitely! I really want to finish this, it’s just my $job that intervened.

stale · 2018-12-26T20:36:43Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

gergelypolonkai · 2018-12-28T07:35:47Z

Given i just got my second “stale” message, i hereby ask for some help. I tried to create a test for the class based skills, and for that i have created a mockedmodule with a skill in it. However, i can’t seem to figure out how to load it. Is there anyone who can give me some pointers?

jacobtomlinson · 2018-12-30T07:22:40Z

You should be able to create an opsdroid instance, preferably using the context manager. Then you have a few options. You could either mock out opsdroid.loader.load_modules_from_config to return a list containing your module, then call opsdroid.load. Or you could add your module to opsdroid.modules and call the pieces of opsdroid.load that you wish to test.

* Bump Pyyaml to avoid CVE-2017-18342 * Add SafeLoader to yaml.load * Add test for fix

…oid#807) * Support custom webhook response * Add tests for custom webhook response * Add docs for custom webhook response

…/opsdroid into gergelypolonkai-class-based-skills

jacobtomlinson · 2019-01-12T16:05:49Z

Well it has taken a couple of months and some back and forth but it looks like this is ready to merge. Thanks for all the hard work @gergelypolonkai. I've managed to find some time today to sit down and sort out the tests and documentation, I understand how hard it can be to find time to pick these things back up again.

Ping me your address via DM on Twitter or Gitter and I'll post you some opsdroid stickers!

jacobtomlinson requested changes Nov 13, 2018

View reviewed changes

opsdroid/parsers/regex.py Outdated Show resolved Hide resolved

opsdroid/skill.py Outdated Show resolved Hide resolved

opsdroid/skill.py Outdated Show resolved Hide resolved

jacobtomlinson requested changes Nov 13, 2018

View reviewed changes

gergelypolonkai force-pushed the class-based-skills branch 3 times, most recently from e950db5 to 528e6f0 Compare November 28, 2018 14:28

jacobtomlinson approved these changes Nov 28, 2018

View reviewed changes

stale bot added the stale label Dec 12, 2018

stale bot removed the stale label Dec 12, 2018

stale bot added the stale label Dec 26, 2018

gergelypolonkai added 8 commits December 28, 2018 08:33

Make it possible to create class-based skills

6391f33

Rework based on requests in the PR

11d8826

Add deprecation warnings if a function based skill is loaded

f75e53c

Update documentation with the details of the class based skills

1cffef5

Add pylint exception and remark about a broad except in skill.py

9f9ca2a

Add tests for the Skill base class

6ceaf50

wip: Add tests for the class-based skill loader

53f8cb6

WIP: Add tests for the class based skills

3a251b5

gergelypolonkai force-pushed the class-based-skills branch from 528e6f0 to 3a251b5 Compare December 28, 2018 07:34

stale bot removed the stale label Dec 28, 2018

pyup-bot and others added 16 commits January 7, 2019 08:45

Update pytest-cov from 2.6.0 to 2.6.1 (opsdroid#799)

e86cb45

Update pillow from 5.3.0 to 5.4.1 (opsdroid#797)

b8f2973

Update pytest from 4.0.2 to 4.1.0 (opsdroid#798)

062599a

Update arrow from 0.12.1 to 0.13.0 (opsdroid#800)

ee76e87

Update recommonmark from 0.4.0 to 0.5.0 (opsdroid#802)

6eeb6ba

Update aiohttp from 3.5.1 to 3.5.2 (opsdroid#801)

5433284

Update aiohttp from 3.5.2 to 3.5.3 (opsdroid#804)

bdd042a

Bump Pyyaml to avoid CVE-2017-18342 (opsdroid#803)

59c2025

* Bump Pyyaml to avoid CVE-2017-18342 * Add SafeLoader to yaml.load * Add test for fix

Update aiohttp from 3.5.3 to 3.5.4 (opsdroid#805)

14e1530

Support custom webhook response (replacement for opsdroid#793) (opsdr…

93f1eb3

…oid#807) * Support custom webhook response * Add tests for custom webhook response * Add docs for custom webhook response

Merge branch 'class-based-skills' of git://github.com/gergelypolonkai…

e7db9cf

…/opsdroid into gergelypolonkai-class-based-skills

Fix linting issues

ed1c6cf

Fix broken test

7401c74

Add tests for class skills

e391849

Update all documentation to use new class based skills

211864a

Update class name

d6e5d7f

jacobtomlinson merged commit e7c49cb into opsdroid:master Jan 12, 2019

jacobtomlinson mentioned this pull request Jan 27, 2019

Call skills correctly depending on function or class skills #820

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make it possible to create class-based skills #734

Make it possible to create class-based skills #734

gergelypolonkai commented Nov 13, 2018 •

edited

jacobtomlinson left a comment

jacobtomlinson left a comment

codecov bot commented Nov 13, 2018 •

edited

gergelypolonkai commented Nov 13, 2018

jacobtomlinson left a comment

jacobtomlinson Nov 13, 2018

gergelypolonkai Nov 13, 2018

jacobtomlinson Nov 13, 2018

gergelypolonkai commented Nov 13, 2018

gergelypolonkai commented Nov 13, 2018

jacobtomlinson commented Nov 13, 2018

jacobtomlinson commented Nov 20, 2018

gergelypolonkai commented Nov 20, 2018

stale bot commented Dec 12, 2018

jacobtomlinson commented Dec 12, 2018

gergelypolonkai commented Dec 12, 2018

stale bot commented Dec 26, 2018

gergelypolonkai commented Dec 28, 2018

jacobtomlinson commented Dec 30, 2018

jacobtomlinson commented Jan 12, 2019

Make it possible to create class-based skills #734

Make it possible to create class-based skills #734

Conversation

gergelypolonkai commented Nov 13, 2018 • edited

Description

Status

Type of change

How Has This Been Tested?

Checklist:

jacobtomlinson left a comment

Choose a reason for hiding this comment

jacobtomlinson left a comment

Choose a reason for hiding this comment

codecov bot commented Nov 13, 2018 • edited

Codecov Report

gergelypolonkai commented Nov 13, 2018

jacobtomlinson left a comment

Choose a reason for hiding this comment

jacobtomlinson Nov 13, 2018

Choose a reason for hiding this comment

gergelypolonkai Nov 13, 2018

Choose a reason for hiding this comment

jacobtomlinson Nov 13, 2018

Choose a reason for hiding this comment

gergelypolonkai commented Nov 13, 2018

gergelypolonkai commented Nov 13, 2018

jacobtomlinson commented Nov 13, 2018

jacobtomlinson commented Nov 20, 2018

gergelypolonkai commented Nov 20, 2018

stale bot commented Dec 12, 2018

jacobtomlinson commented Dec 12, 2018

gergelypolonkai commented Dec 12, 2018

stale bot commented Dec 26, 2018

gergelypolonkai commented Dec 28, 2018

jacobtomlinson commented Dec 30, 2018

jacobtomlinson commented Jan 12, 2019

gergelypolonkai commented Nov 13, 2018 •

edited

codecov bot commented Nov 13, 2018 •

edited