Skip to content

Fuzzy match the role command.#1202

Merged
MarkKoz merged 6 commits into
masterfrom
fuzz-the-matches
Jan 20, 2021
Merged

Fuzzy match the role command.#1202
MarkKoz merged 6 commits into
masterfrom
fuzz-the-matches

Conversation

@kosayoda
Copy link
Copy Markdown
Contributor

@kosayoda kosayoda commented Oct 1, 2020

Resolves #1194.

  • The !role command now fuzzy matches.
    image

  • Duplicates are filtered.

An arbitrary cutoff score of 80 is chosen because it works. A bug in
the test for the same command is also fixed.
This prevents weird fuzz matches like `!role a b c d` working.
@kosayoda kosayoda added t: feature New feature or request a: frontend Related to output and formatting p: 2 - normal Normal Priority a: information Related to information commands: (doc, help, information, reddit, site, tags) labels Oct 1, 2020
@kosayoda kosayoda requested a review from a team as a code owner October 1, 2020 05:29
@kosayoda kosayoda requested review from Den4200 and scragly and removed request for a team October 1, 2020 05:29
@ghost ghost added the needs 2 approvals label Oct 1, 2020
Copy link
Copy Markdown
Contributor

@MarkKoz MarkKoz left a comment

Choose a reason for hiding this comment

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

It'd be nice for it to work on individual words e.g. events for events team without just reducing the threshold. That's probably too complicated. This is still fine.

Copy link
Copy Markdown
Member

@Den4200 Den4200 left a comment

Choose a reason for hiding this comment

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

Everything looks and works fine!

The only thing is the output of roles are not in the given order because of the switch to sets. It's nothing major, but I'll let you decide if we want to keep the sets or switch back to lists.

@ghost ghost removed the needs 1 approval label Oct 2, 2020
Copy link
Copy Markdown
Contributor

@kwzrd kwzrd left a comment

Choose a reason for hiding this comment

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

Works just fine; have you considered also trying a direct substring search? That would allow for e.g. "Core Dev" to match, but it'd only make sense if the search term is n chars long. Perhaps needlessly complex, as Mark says.

Comment thread bot/exts/info/information.py
@Xithrius
Copy link
Copy Markdown
Contributor

Xithrius commented Jan 17, 2021

@kosayoda we will be able to merge after you fix bot building errors and file conflicts.

Please finish this off when you're available.

@coveralls
Copy link
Copy Markdown

coveralls commented Jan 18, 2021

Coverage Status

Coverage increased (+0.02%) to 56.742% when pulling dbb2a6b on fuzz-the-matches into eccbebf on master.

@kosayoda
Copy link
Copy Markdown
Contributor Author

sigh forgot to pull master before merging, at least it's not conflict inducing

@MarkKoz MarkKoz merged commit 04c3a18 into master Jan 20, 2021
@MarkKoz MarkKoz deleted the fuzz-the-matches branch January 20, 2021 23:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

a: frontend Related to output and formatting a: information Related to information commands: (doc, help, information, reddit, site, tags) p: 2 - normal Normal Priority t: feature New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add fuzzy matching to the role(s) command.

7 participants