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

adminchannel: empty .tmask handled incorrectly #2596

Closed
dgw opened this issue Mar 6, 2024 · 3 comments · Fixed by #2601
Closed

adminchannel: empty .tmask handled incorrectly #2596

dgw opened this issue Mar 6, 2024 · 3 comments · Fixed by #2601
Labels
Bug Things to squish; generally used for issues
Milestone

Comments

@dgw
Copy link
Member

dgw commented Mar 6, 2024

Description

Running .tmask without any argument technically sets an empty topic mask, but in a broken way.

Reproduction steps

  1. Enable adminchannel plugin if not enabled
  2. As chanop, do just .tmask (no argument)
  3. Do .showmask
  4. Sopel responds with an exception:
    Unexpected TypeError (can only concatenate str (not "NoneType") to str) from dgw. Message was: .showmask

Expected behavior

adminchannel should properly clear the topic mask in this case, such that doing .showmask yields the same output as if .tmask had never been set at all.

Additionally, .showmask should handle None as if the topic mask is unset. Defense in depth!

Relevant logs

No response

Notes

I've a half-assed patch for this in adminchannel-tmask-none branch, but it's untested and probably needs some more work before becoming a pull request.

Sopel version

973a489

Installation method

pip install

Python version

3.10.7

Operating system

No response

IRCd

No response

Relevant plugins

adminchannel

@dgw dgw added the Bug Things to squish; generally used for issues label Mar 6, 2024
@dgw dgw added this to the 8.0.1 milestone Mar 6, 2024
@half-duplex
Copy link
Member

Unless we have an established pattern, I prefer possibly-destructive things be explicit: an argument of empty quotes, a hyphen, -clear, or similar to clear it, .tmask without arguments shows usage including how to clear.

@dgw
Copy link
Member Author

dgw commented Mar 6, 2024

The thought of adding a .clearmask or .tmclear command did cross my mind, yes. I had forgotten about .showmask, didn't expect .tmask to accept an empty arg either, and was surprised.

@dgw dgw modified the milestones: 8.0.1, 8.0.0 Mar 18, 2024
@dgw
Copy link
Member Author

dgw commented Mar 18, 2024

Process note: We're likely to slot #2601 into 8.0.0 because it's a user-facing change and not a pure bugfix, so I've changed the issue's target milestone.

@dgw dgw closed this as completed in #2601 Mar 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Things to squish; generally used for issues
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants