plugins: check consistency of plugin code and their output formatting #1937

Exirel · 2020-09-12T15:02:46Z

Description

I'm trying to fix, at least partially, issue #709 which turned out differently than what I expected.

So in the end, I also checked the code's consistency, and decided to:

use bot.reply in case of error
remove if main block (they are pointless)
replace sopel.module by sopel.plugin; we know it works, it's fine, let's move on with our life
replace return bot.reply statement because a one-liner that makes no sense is harmful to beginner who might not understand what we are trying to achieve with it; also the code should express the intent, it is not a showcase of how smart you are at reducing the file length (mood: 😠)
curse in French (a lot)
normalize import as much as possible (using from sopel import plugin instead of anything else)
a lot of cursing in French actually (:fr: :cursing_face:)
~~completely ignore~~ eventually did the meetbot plugin, and that one can go 🇫🇷 🤬 itself
not fix the tld plugin, it looks complicated and I hate it—go read on Wikipedia you lazy nerd!
listening to darksynth/EDM music, that has nothing to do with Sopel but it was necessary while doing that

If anyone complain or have comments or suggestion: this PR is here to fix #709 so you too can help, so please do!

Checklist

I have read CONTRIBUTING.md
I can and do license this contribution under the EFLv2
No issues are reported by make qa (runs make quality and make test)
I have tested the functionality of the things this change touches

lgtm-com · 2020-09-12T15:15:54Z

This pull request introduces 1 alert when merging b4961a8 into 528e1b8 - view on LGTM.com

new alerts:

1 for Module is imported with 'import' and 'import from'

dgw

I have a vested interest in getting this done and merged soon, since I built another PR on top of it to save on rebasing work later. So I hope you'll not be too annoyed by my suggestions here, and we can get this whipped into shape faster than that interminable plugin-chapter doc PR we just finished. 😹

sopel/modules/adminchannel.py

sopel/modules/announce.py

sopel/modules/calc.py

sopel/modules/clock.py

sopel/modules/uptime.py

sopel/modules/url.py

sopel/modules/version.py

sopel/modules/wikipedia.py

sopel/modules/xkcd.py

dgw · 2020-09-14T02:43:13Z

Whoops, forgot to check LGTM's comment before submitting that review. It has a point. find_updates.py is kind of awkward now with the overlapping imports.

lgtm-com · 2020-09-14T07:28:31Z

This pull request introduces 1 alert when merging 0b471de into 58bd737 - view on LGTM.com

new alerts:

1 for Module is imported with 'import' and 'import from'

sopel/modules/search.py

Exirel · 2020-09-14T10:22:31Z

I'll hate the squash & rebase that will come. And all the potential merge conflict to come.

Oh well, it has to be done.

@dgw waiting on your approval.

dgw

Once I managed to coax a "changes since last review" view out of GitHub (only accessible from the Conversation tab, for some reason; the Files tab dropdown had that option greyed out), this was a pretty easy re-review. Just a couple little things, and one additional can of worms opened accidentally by the added tweaks to those few command docstrings that started with """.… 😅

sopel/modules/xkcd.py

sopel/modules/ip.py

sopel/modules/uptime.py

dgw

Between the last little tweaks and the follow-up items added to my to-do list, this should be good to go. Squash when ready!

Duckle29

Only looked at currency, but LGTM 👍

dgw · 2020-09-14T23:06:37Z

@Duckle29 thanks for the extra eyes! 🙌

* replaced `sopel.module` by `sopel.plugin` * replaced `reply` by `say` in private message only commands

* replaced `sopel.module` by `sopel.plugin` * replaced duplicated text by a constant * replaced `return bot.reply` by 2 lines Note: `bot.reply`'s return has no meaning. Using it in a return statement creates confusion as to why it is used in such a way. Therefore, it should **not** be used that way.

* replaced `sopel.module` by `sopel.plugin` * added output prefix

* replaced `sopel.module` by `sopel.plugin` * used output prefix with `bot.say` * removed obsolete "if main" block

* replaced `sopel.module` by `sopel.plugin` * removed obsolete "if main" block

* replaced `sopel.module` by `sopel.plugin` * used output prefix with `bot.say` * replaced common error messages by constants

* replaced `sopel.module` by `sopel.plugin` * used output prefix with `bot.say`

* replaced `sopel.module` by `sopel.plugin` * used output prefix with `bot.say` * removed `return plugin.NOLIMIT` from `exchange` function as this return value is not used by its callers

* replaced `sopel.module` by `sopel.plugin` * used output prefix with `bot.say` * removed obsolete "if main" block

* replaced `sopel.module` by `sopel.plugin` * no output prefix for emotes, they are too pure!

* replaced `sopel.module` by `sopel.plugin`

* replaced `sopel.module` by `sopel.plugin` * add manual output prefix in job

* replaced `sopel.module` by `sopel.plugin` * used `plugin.output_prefix` with `bot.say` method * replaced `Likes: N` by `N ♥`` with proper singular/plural * replaced `Comments: N` by `N comment` with proper singular/plural

* replaced `sopel.module` by `sopel.plugin` * replaced `return bot.reply` by 2 lines

* replaced `sopel.module` by `sopel.plugin` * replaced `bot.say` by `bot.reply` for error cases * replaced `return bot.reply` by 2 lines * removed obsolete `if main` block

* replaced `sopel.module` by `sopel.plugin` * removed obsolete `if main` block

* replaced `sopel.module` by `sopel.plugin` * replaced `bot.say` by `bot.reply` for error cases * replaced `return bot.reply` by 2 lines * used `plugin.output_prefix` with `bot.say` method

* replaced `sopel.module` by `sopel.plugin` Note: I didn't use bot.say as .in and .at inform the user that their action succeeded *for them*. It's a very specific case and I didn't feel like using `bot.say` here.

* replaced `sopel.module` by `sopel.plugin` * used `plugin.output_prefix` with `bot.say` method

* replaced `sopel.module` by `sopel.plugin` * replaced `return bot.reply` by 2 lines * used `plugin.output_prefix` with `bot.say` method

* replaced `sopel.module` by `sopel.plugin` * used `plugin.output_prefix` with `bot.say` method

* replaced `sopel.module` by `sopel.plugin` * replaced `return bot.reply` by 2 lines * used `plugin.output_prefix` with `bot.say` method

* replaced `sopel.module` by `sopel.plugin` * used `plugin.output_prefix` with `bot.say` method Note: apparently TLD parsing is broken. This won't fix it.

* replaced `sopel.module` by `sopel.plugin` * replaced `return bot.reply` by 2 lines * used `plugin.output_prefix` with `bot.say` method

* replaced `sopel.module` by `sopel.plugin` * used `plugin.output_prefix` with `bot.say` method * removed obsolete `if main` block

* replaced `sopel.module` by `sopel.plugin` * used `plugin.output_prefix` with `bot.say` method

* replaced `sopel.module` by `sopel.plugin` * used `plugin.output_prefix` with `bot.say` method * removed obsolete `if main` block

* replaced `sopel.module` by `sopel.plugin` * used `plugin.output_prefix` with `bot.say` method

* replaced `sopel.module` by `sopel.plugin` * used `plugin.output_prefix` with `bot.say` method And in theory, I'm done!

Exirel · 2020-09-15T07:28:10Z

Squashed, rebased: 🚢

dgw · 2020-09-16T05:29:00Z

completely ignore the meetbot plugin, this one can go 🇫🇷 🤬 itself

In the end, this was a lie, since you did touch it after all. 😹

not fix the tld plugin, it looks complicated and I hate it—go read on Wikipedia you lazy nerd!

But saying this motivated someone else (me) to work on it, with a PR coming Soon™ to a repo near you! (Pro tip: It was a lot less complicated to just rewrite the whole thing with a different strategy than to fix the regex. 😁)

reddit: remove stray hard-coded message prefix missed in #1937

Exirel added the Tweak label Sep 12, 2020

Exirel added this to the 7.1.0 milestone Sep 12, 2020

dgw self-requested a review September 13, 2020 11:02

dgw requested changes Sep 14, 2020

View reviewed changes

Exirel commented Sep 14, 2020

View reviewed changes

sopel/modules/search.py Show resolved Hide resolved

dgw reviewed Sep 14, 2020

View reviewed changes

sopel/modules/xkcd.py Outdated Show resolved Hide resolved

sopel/modules/ip.py Show resolved Hide resolved

sopel/modules/uptime.py Show resolved Hide resolved

dgw approved these changes Sep 14, 2020

View reviewed changes

Duckle29 approved these changes Sep 14, 2020

View reviewed changes

Exirel added 17 commits September 15, 2020 09:14

admin: check output consistency

ed782e9

* replaced `sopel.module` by `sopel.plugin` * replaced `reply` by `say` in private message only commands

announce: check output consistency

0141509

* replaced `sopel.module` by `sopel.plugin` * added output prefix

bugzilla: proper config import

eea3e4a

calc: check output consistency

9279381

* replaced `sopel.module` by `sopel.plugin` * used output prefix with `bot.say` * removed obsolete "if main" block

choose: check output consistency

1c689d1

* replaced `sopel.module` by `sopel.plugin` * removed obsolete "if main" block

clock: check output consistency

9d7d8ee

* replaced `sopel.module` by `sopel.plugin` * used output prefix with `bot.say` * replaced common error messages by constants

countdown: check output consistency

48e3f30

* replaced `sopel.module` by `sopel.plugin` * used output prefix with `bot.say`

currency: check output consistency

652c6bd

* replaced `sopel.module` by `sopel.plugin` * used output prefix with `bot.say` * removed `return plugin.NOLIMIT` from `exchange` function as this return value is not used by its callers

dice: check output consistency

0c9d5af

* replaced `sopel.module` by `sopel.plugin` * used output prefix with `bot.say` * removed obsolete "if main" block

emoticons: check output consistency

ab0cac7

* replaced `sopel.module` by `sopel.plugin` * no output prefix for emotes, they are too pure!

find: check output consistency

efa2798

* replaced `sopel.module` by `sopel.plugin`

find_updates: check output consistency

f7695ab

* replaced `sopel.module` by `sopel.plugin` * add manual output prefix in job

help: replace sopel.module by sopel.plugin

4d299d1

instagram: check output consistency

c92eba9

* replaced `sopel.module` by `sopel.plugin` * used `plugin.output_prefix` with `bot.say` method * replaced `Likes: N` by `N ♥`` with proper singular/plural * replaced `Comments: N` by `N comment` with proper singular/plural

invite: check output consistency

e213bd5

* replaced `sopel.module` by `sopel.plugin` * replaced `return bot.reply` by 2 lines

ip: check output consistency

8b6318f

* replaced `sopel.module` by `sopel.plugin` * replaced `bot.say` by `bot.reply` for error cases * replaced `return bot.reply` by 2 lines * removed obsolete `if main` block

Exirel added 19 commits September 15, 2020 09:17

py: check output consistency

72da978

* replaced `sopel.module` by `sopel.plugin` * removed obsolete `if main` block

rand: check output consistency

8b0e497

* replaced `sopel.module` by `sopel.plugin` * removed obsolete `if main` block

reddit: check output consistency

fa9516a

* replaced `sopel.module` by `sopel.plugin` * replaced `bot.say` by `bot.reply` for error cases * replaced `return bot.reply` by 2 lines * used `plugin.output_prefix` with `bot.say` method

reload: check output consistency

67e5656

* replaced `sopel.module` by `sopel.plugin` * replaced `bot.say` by `bot.reply` for error cases * replaced `return bot.reply` by 2 lines * used `plugin.output_prefix` with `bot.say` method

remind: check output consistency

9094e8b

* replaced `sopel.module` by `sopel.plugin` Note: I didn't use bot.say as .in and .at inform the user that their action succeeded *for them*. It's a very specific case and I didn't feel like using `bot.say` here.

safety: check output consistency

5f9e014

* replaced `sopel.module` by `sopel.plugin` * used `plugin.output_prefix` with `bot.say` method

search: check output consistency

e7ad8ba

* replaced `sopel.module` by `sopel.plugin` * replaced `return bot.reply` by 2 lines * used `plugin.output_prefix` with `bot.say` method

seen: check output consistency

3d9a540

* replaced `sopel.module` by `sopel.plugin` * used `plugin.output_prefix` with `bot.say` method

tell: check output consistency

b71fa31

* replaced `sopel.module` by `sopel.plugin` * replaced `return bot.reply` by 2 lines * used `plugin.output_prefix` with `bot.say` method

tld: check output consistency

b71063e

* replaced `sopel.module` by `sopel.plugin` * used `plugin.output_prefix` with `bot.say` method Note: apparently TLD parsing is broken. This won't fix it.

translate: check output consistency

aae4c81

* replaced `sopel.module` by `sopel.plugin` * replaced `return bot.reply` by 2 lines * used `plugin.output_prefix` with `bot.say` method

unicode_info: check output consistency

0e5ee11

* replaced `sopel.module` by `sopel.plugin` * used `plugin.output_prefix` with `bot.say` method * removed obsolete `if main` block

units: check output consistency

e875c63

* replaced `sopel.module` by `sopel.plugin` * used `plugin.output_prefix` with `bot.say` method * removed obsolete `if main` block

uptime: check output consistency

f156e8c

* replaced `sopel.module` by `sopel.plugin` * used `plugin.output_prefix` with `bot.say` method

url: check output consistency

278ad8e

* replaced `sopel.module` by `sopel.plugin` * used `plugin.output_prefix` with `bot.say` method * removed obsolete `if main` block

version: check output consistency

4188154

* replaced `sopel.module` by `sopel.plugin` * used `plugin.output_prefix` with `bot.say` method

wikipedia: check output consistency

85535b0

* replaced `sopel.module` by `sopel.plugin` * used `plugin.output_prefix` with `bot.say` method

wiktionary: check output consistency

eb02803

* replaced `sopel.module` by `sopel.plugin` * used `plugin.output_prefix` with `bot.say` method

xkcd: check output consistency

ce5cf44

* replaced `sopel.module` by `sopel.plugin` * used `plugin.output_prefix` with `bot.say` method And in theory, I'm done!

Exirel force-pushed the consistency-plugins-output branch from 18bf093 to ce5cf44 Compare September 15, 2020 07:25

dgw referenced this pull request Sep 16, 2020

build: pin coveralls requirement on old Python versions

c9d721c

dgw merged commit 19522fc into sopel-irc:master Sep 16, 2020

dgw added a commit that referenced this pull request Sep 25, 2020

reddit: remove stray hard-coded message prefix missed in #1937

2009dbf

This was referenced Sep 25, 2020

reddit: append subreddit information with sorting methods #1883

Closed

reddit: remove stray hard-coded message prefix missed in #1937 #1954

Merged

find_updates: restore unstable release handling #1955

Merged

dgw added a commit that referenced this pull request Nov 22, 2020

Merge pull request #1954 from sopel-irc/reddit-missed-hard-prefix

36c6a59

reddit: remove stray hard-coded message prefix missed in #1937

Exirel deleted the consistency-plugins-output branch May 1, 2021 17:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

plugins: check consistency of plugin code and their output formatting #1937

plugins: check consistency of plugin code and their output formatting #1937

Exirel commented Sep 12, 2020 •

edited

Loading

lgtm-com bot commented Sep 12, 2020

dgw left a comment

dgw commented Sep 14, 2020

lgtm-com bot commented Sep 14, 2020

Exirel commented Sep 14, 2020

dgw left a comment

dgw left a comment

Duckle29 left a comment

dgw commented Sep 14, 2020

Exirel commented Sep 15, 2020

dgw commented Sep 16, 2020

plugins: check consistency of plugin code and their output formatting #1937

plugins: check consistency of plugin code and their output formatting #1937

Conversation

Exirel commented Sep 12, 2020 • edited Loading

Description

Checklist

lgtm-com bot commented Sep 12, 2020

dgw left a comment

Choose a reason for hiding this comment

dgw commented Sep 14, 2020

lgtm-com bot commented Sep 14, 2020

Exirel commented Sep 14, 2020

dgw left a comment

Choose a reason for hiding this comment

dgw left a comment

Choose a reason for hiding this comment

Duckle29 left a comment

Choose a reason for hiding this comment

dgw commented Sep 14, 2020

Exirel commented Sep 15, 2020

dgw commented Sep 16, 2020

Exirel commented Sep 12, 2020 •

edited

Loading