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

choose: safely handle formatted options #1965

Merged
merged 7 commits into from
Dec 10, 2020
Merged

Conversation

dgw
Copy link
Member

@dgw dgw commented Oct 19, 2020

Description

Briefly, this resolves #1877. No longer will unterminated formatting, or leading/trailing formatting characters, cause formatting issues in the .choose command's output.

The custom "strip"/"cleanup" function that makes it all possible comes replete with 16 20 23 passing cases and 8 expect-fail cases for future enhancements—though I'm sure there are weird cases I haven't covered with the existing test params. Suggestions welcome, and/or we can just see what breaks once this hits the wild.

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

dgw added 2 commits October 18, 2020 21:39
This is for plugin convenience, as otherwise they'd all need to build
their own lists of constants from the formatting module. Providing a
constant also lets plugins adapt automatically when Sopel adds support
for new formatting characters.
@dgw dgw added the Bugfix Generally, PRs that reference (and fix) one or more issue(s) label Oct 19, 2020
@dgw dgw added this to the 7.1.0 milestone Oct 19, 2020
@dgw dgw requested a review from a team October 19, 2020 05:06
@dgw
Copy link
Member Author

dgw commented Oct 19, 2020

Wow, I really whiffed that. Was so focused on making sure my new tests behaved that I neglected the existing ones.

@dgw dgw marked this pull request as draft October 19, 2020 05:15
@dgw dgw force-pushed the choose-preserve-formatting branch from da02739 to 967f747 Compare October 19, 2020 05:41
dgw added 2 commits October 19, 2020 00:50
Python's built-in `strip()` method for strings considers some control
codes used for IRC formatting to be "whitespace" and removes them if
they appear at the beginning or end of the string. While the built-in
method does take an optional argument listing what characters to strip,
there's no good way to generate our own list to pass in. One can look up
the category of a Unicode character, but there's no way to get a list of
all characters in a category.

So instead, we have a custom function, which also handles adding the
"formatting reset" character if the text contains any formatting. (My
original idea was to write a "parser" of sorts that would insert the
appropriate control code to "close" unmatched formatting, but then I
remembered that the "reset" 0x0F exists and it was Good Enough™.)

Future revisions could gain a little more efficiency on the wire by only
adding the reset character if the text contains an unmatched formatting
code (but that quickly devolves into diminishing returns, so it's likely
not worth doing unless someone gets *really* bored).
@dgw dgw force-pushed the choose-preserve-formatting branch from 967f747 to 078c4ac Compare October 19, 2020 05:50
@dgw dgw marked this pull request as ready for review October 19, 2020 05:50
@dgw
Copy link
Member Author

dgw commented Oct 19, 2020

Should be all fixed, or at least the full make test suite passes locally. Got distracted by sopel-irc/sopel-github#77 first.

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.

As far as I'm concerned, it works and it does the job, so good for me.

Well OK yes I suggested some changes, but they are pretty minor.

sopel/modules/choose.py Outdated Show resolved Hide resolved
sopel/modules/choose.py Show resolved Hide resolved
Copy link
Member Author

@dgw dgw left a comment

Choose a reason for hiding this comment

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

In which @dgw declines to lazily apply @Exirel's suggestions. 😁

sopel/modules/choose.py Show resolved Hide resolved
sopel/modules/choose.py Outdated Show resolved Hide resolved
Co-authored-by: Exirel <florian.strzelecki@gmail.com>
@dgw
Copy link
Member Author

dgw commented Oct 19, 2020

I'm going to stop messing with this now. 23 passing + 8 xfail test cases is enough for this one silly little function. 😛

@dgw dgw force-pushed the choose-preserve-formatting branch from 0c763cf to c89fa55 Compare October 23, 2020 03:07
Because Python doesn't have a static type system.
@dgw dgw force-pushed the choose-preserve-formatting branch from c89fa55 to 582baad Compare October 23, 2020 04:08
@dgw
Copy link
Member Author

dgw commented Oct 23, 2020

I'm going to stop messing with this now.

Yeah, that didn't happen. But I'm really done now. CI passed on all versions this time!

@dgw dgw merged commit e996b87 into master Dec 10, 2020
@dgw dgw deleted the choose-preserve-formatting branch December 10, 2020 04:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bugfix Generally, PRs that reference (and fix) one or more issue(s)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

choose: IRC formatting can make weird output
2 participants