-
-
Notifications
You must be signed in to change notification settings - Fork 409
url: urlexclude/allow commands; auto-title ignores URLs in ANY other command #2028
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
Conversation
|
I like this, my only suggestion is what do you think about renaming the commands to use a different word than "ban"? At least to me using "ban" implies banning someone from using the plugin. I know it also includes "url" in the command name, so perhaps it is less confusing to others, but the plugin is also named |
|
If you have something better than |
|
Yes sure, exclude makes more sense to me |
dgw
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"urlpunban" lol 😹
I couldn't take the time just now to think about the # re-compile steps, but surely there's a way to manage bot.memory['url_exclude'] without running a whole list comprehension every time?
I think there is, but I couldn't bother to do something complicated without the proper unit-test to back me up. I think the tradeoff is good for now. I know I tend to be a bit lazy with tests when it comes to plugin, because I never know if I really want to invest in them right now, or wait for when they are all external plugins. After all, I did a 100% test coverage on sopel-help, and I'm doing that for remind too... |
dgw
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Squash at will, sir. 🖖
Per IRC discussion, the fixup doesn't work as expected due to sopel.config.types.ListAttribute shenanigans.
f03f3b2 to
fc5f7bc
Compare
|
OK, I squashed and rebased after some further fix and changes, making it worthy of a second full review, I think. I've settled for |
dgw
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Retitled to reflect the extra fix Exi sneakily added, so I remember to mention it in the changelog.
Description
Close #1870
And because nothing is simple: I also added aliases for
urlpbanandurlpunbanto ban/unban patterns instead of URL directly.Checklist
make qa(runsmake qualityandmake test)