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

irc: more ways for bot.say() to help with overlength messages #2050

Merged
merged 5 commits into from
Apr 29, 2021

Conversation

dgw
Copy link
Member

@dgw dgw commented Apr 2, 2021

Description

Addresses a shortcoming of the trailing parameter, which alone is insufficient to handle some common output patterns used by Sopel's own core plugins.

For example, the wikipedia plugin outputs article snippets in between quotation marks. Long snippets are truncated and the quote closed by the trailing parameter, but short snippets never invoke trailing. They lose both the closing quote AND the URL linking to the full article.

With these now two optional parameters (truncation and trailing), it will be easier for plugins to include variable-length data in "the middle" of a message and still guarantee that any important information at the end of the line will be shown.

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

Notes

The name finial is the best idea I could think up while writing this patch, but there's probably a better option out there. What I want is finally, but that's a reserved word… Replaced "finial" with "trailing", and renamed the existing "trailing" parameter to "truncation" (short for "truncation indicator").

When writing the new parameter, I thought more core plugins would use it. Looks like only wikipedia needs the extra finesse, but that's fine. I have a few external plugins that will take advantage.

And yes, I'm fully aware that this approaches over-engineering. 🙃

dgw added 4 commits April 1, 2021 02:17
Addresses a shortcoming of the `trailing` parameter, which alone is
insufficient to handle some common output patterns used by Sopel's own
core plugins.

For example, the `wikipedia` plugin outputs article snippets in between
quotation marks. Long snippets are truncated and the quote closed by the
`trailing` parameter, but short snippets never invoke `trailing`. They
lose both the closing quote AND the URL linking to the full article.

With this additional parameter, it will be easier for plugins to include
variable-length data in "the middle" of a message and still guarantee
that any important information at the end of the line will be shown.
Fixes a bug that I never reported in an issue: Missing the URL on short
snippets because the URL was part of `trailing`, which only gets used if
the text is too long for an IRC line.

That bug was the whole inspiration for implementing a new parameter.

The only outstanding problem I see now is that a short first line can
make it appear as if the article ends there:

    [wikipedia] ONT | "ONT or Ont may refer to:" | <link_to_page>

Ideal output would be:

    [wikipedia] ONT | "ONT or Ont may refer to: […]" | <link_to_page>

That's a separate issue, though.
@dgw dgw added the Feature label Apr 2, 2021
@dgw dgw added this to the 7.1.0 milestone Apr 2, 2021
@dgw dgw requested a review from a team April 2, 2021 05:07
@Exirel
Copy link
Contributor

Exirel commented Apr 2, 2021

I didn't really understand the usefulness of finial until I saw your usage in wikipedia: it's great when the finial argument is not just a quote mark, but a URL. Maybe I would suggest to tweak the docstring of bot.say to include that information, it feels like the right use-case.

@dgw
Copy link
Member Author

dgw commented Apr 2, 2021

Maybe I would suggest to tweak the docstring of bot.say to include that information

Like this? "It's useful for":

sopel/sopel/irc/__init__.py

Lines 570 to 571 in fb38d3b

making sure e.g. a link is always included, even if the summary your
plugin fetches is too long to fit.


Meanwhile I still don't really like the name "finial". 🙁 Maybe I should just make it "final". Or, since I can't use "finally" because it's a reserved word, I briefly consulted a thesaurus and found "lastly" instead. Surely one of these is better than what I wrote at 1am. 😹

@Exirel
Copy link
Contributor

Exirel commented Apr 2, 2021

Surely one of these is better than what I wrote at 1am. 😹

Maybe just output_suffix?

@dgw
Copy link
Member Author

dgw commented Apr 2, 2021

I thought of and immediately discarded the idea of "suffix" as too similar to the plugin.output_prefix decorator while writing that comment. 😂 But it's honestly still better than "finial"…

Bit of history could be helpful here. I started out renaming the existing trailing parameter to truncated, and used trailing again for this new arg. Somewhere along the way I screwed up some logic and made the existing tests fail, despite renaming all the stuff. After a git reset, in the second version of this patch, I decided to only add a new parameter and sort out the names later.

And that's how I got "finial". It was kind of just a placeholder. Since trailing hasn't been released yet, its name could change too and no one would be upset.

What do you think of renaming trailing -> truncation and finial -> trailing?

@Exirel
Copy link
Contributor

Exirel commented Apr 2, 2021

Oh. It wasn't in 7.0?

What do you think of renaming trailing -> truncation and finial -> trailing?

Oh yes, 100% agree, it makes so much more sense to me! Do you need help with the tests?

@dgw
Copy link
Member Author

dgw commented Apr 2, 2021

Do you need help with the tests?

I'll find out tonight (my time)/tomorrow (your time) when I actually make the change. It should be as easy as making sure I run two find-and-replaces in the correct order… But you should feel free to suggest/add more tests for edge cases I didn't think of!

@dgw dgw changed the title irc: add finial parameter to bot.say() irc: more ways for bot.say() to help with overlength messages Apr 3, 2021
The new-to-7.1 `trailing` parameter is now called `truncation`, so the
new `finial` argument could be renamed to `trailing`.

Updated existing uses of these arguments in core plugins:

  * reddit
  * tld
  * wikipedia
  * wiktionary
@dgw dgw force-pushed the bot.say-better-trailing branch from 62174cb to 400dd57 Compare April 3, 2021 01:19
@dgw
Copy link
Member Author

dgw commented Apr 3, 2021

Renames done. I already broke some stuff by forgetting to update some usage, so double-checking that I didn't miss any previously updated code that expects the old meaning of trailing would be much appreciated!

@Exirel
Copy link
Contributor

Exirel commented Apr 9, 2021

I was looking at all the place where .say( is used, and I think the xkcd plugin could take advantage of the max_message (maybe something set to 3?) with a truncation & trailing with the URL (when the URL parsing is "commanded").

Copy link
Contributor

@Exirel Exirel left a comment

Choose a reason for hiding this comment

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

So, even if you don't update the xkcd plugin, I think it's all good. I checked on my side if I could see anything missing, and it looks all good to me!

@dgw dgw merged commit 057043a into master Apr 29, 2021
@dgw dgw deleted the bot.say-better-trailing branch April 29, 2021 02:30
@dgw
Copy link
Member Author

dgw commented Apr 29, 2021

I have elected to leave the xkcd plugin alone, so as not to alter its behavior in an unrelated PR. The other plugins touched here were either bug fixes or no-op replacement of logic without changing behavior. I think… merged this from my phone, haha.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants