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

Add parameter to configure priority in every regex skill (proposal) #692

Merged
merged 3 commits into from
Oct 8, 2018

Conversation

anxodio
Copy link
Contributor

@anxodio anxodio commented Oct 5, 2018

Description

Here I am again! Hacktoberfest motivated me :)

So I'm recovering an old discussion that was gone stale, I read it again, and I wrote this changes that I think would make all of us happy.

The point here is change the concept of REGEX_MAX_SCORE to REGEX_SCORE_FACTOR (in fact, it was a factor all the time, not a max score), add an optional keyword argument in match_regex to empower the developer to change this factor, but if not provided will be the configured REGEX_SCORE_FACTOR (stills in 0.6).

With this change, we let the developers prioritize skills that they want to be executed over the NLU skills.

It's a proposal, so if contributors are happy with it, I will update the docs before merging the PR.

Fixes #458

Status

UNDER DEVELOPMENT

Type of change

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

How Has This Been Tested?

  • TestParserRegex.test_parse_regex_priority: it registers the same regex with different factors and ensures the skill with higher priority is executed.

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

@codecov
Copy link

codecov bot commented Oct 5, 2018

Codecov Report

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

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #692   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          24     24           
  Lines        1521   1522    +1     
=====================================
+ Hits         1521   1522    +1
Impacted Files Coverage Δ
opsdroid/const.py 100% <100%> (ø) ⬆️
opsdroid/matchers.py 100% <100%> (ø) ⬆️
opsdroid/parsers/regex.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 1bce0cc...7ce7e80. Read the comment docs.

@jacobtomlinson
Copy link
Member

This is really great! Thanks for getting back into it.

I'm happy with this approach, it should help us progress with matchers like the parser one you started previously.

@anxodio
Copy link
Contributor Author

anxodio commented Oct 8, 2018

Nice! So I added some explanation in docs, please review it (my English structures can fail sometimes) 😄


In order to make NLU skills execute over regex skills, opsdroid always applies a factor of `0.6` to every regex evaluated score.

Sometimes can be interesting for a developer to get a regex skill executed over a NLU skill, and `score_factor` keyword argument can be used to archieve this.
Copy link
Member

Choose a reason for hiding this comment

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

I would restructure this sentence like this:
"If a developer want to have a regex skill executed over a NLU one then the keyword argument score_factor can be used to achieve this."

@FabioRosado
Copy link
Member

Hey @anxodio thanks for updating the documentation so quickly. I am going to merge this PR now 😄 👍

@FabioRosado FabioRosado merged commit 233a951 into opsdroid:master Oct 8, 2018
@anxodio anxodio deleted the regex_priority branch October 8, 2018 13:53
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.

Change the way opsdroid prioritize regex / NLU skills
3 participants