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: rework topic-mask commands #2601

Merged
merged 4 commits into from
Mar 20, 2024
Merged

Conversation

dgw
Copy link
Member

@dgw dgw commented Mar 18, 2024

Description

  • .tmask get shows the current topic mask
    • .tmask without arguments assumes the get subcommand
  • .tmask set now sets the topic mask, and provides a usage hint if called without further arguments
  • .tmask clear is added to explicitly clear the topic mask

In combination, the above should be sufficient to resolve #2596.

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 lint and make test)
  • I have tested the functionality of the things this change touches

Notes

If no one minds adding this to 8.0.0 while the release notes are still under peer review, I'm happy to get it shipped sooner.

(Because it's modifying command behavior, if we don't ship this patch in 8.0.0, I believe the semver-appropriate thing to do would be to ship the first commit in 8.0.1 and hold the second for 8.1.0.)

dgw added 2 commits March 5, 2024 20:36
Bonus code style fix using `default` arg to `bot.db.get_channel_value()`
- `.tmask` without arguments now shows the current topic mask instead of
  clearing it.
- `.showmask` command is redundant with the above, and has been removed.
- `.cleartmask` is added to explicitly clear the topic mask.
@dgw dgw added Tweak Bugfix Generally, PRs that reference (and fix) one or more issue(s) labels Mar 18, 2024
@dgw dgw added this to the 8.0.0 milestone Mar 18, 2024
@dgw dgw requested a review from a team March 18, 2024 00:13
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.

I'm all for it. I made an optional suggestion for a "ctmask" alias.

sopel/builtins/adminchannel.py Outdated Show resolved Hide resolved
@dgw
Copy link
Member Author

dgw commented Mar 19, 2024

Refactored to use subcommands. I kind of like this style, actually. The .help situation is a bit messy, though the details of that would be a discussion for the sopel-help issue tracker.

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.

Nitpicking aside, LGTM.

sopel/builtins/adminchannel.py Outdated Show resolved Hide resolved
@dgw
Copy link
Member Author

dgw commented Mar 20, 2024

Last night, @half-duplex said in our IRC: "reading only the strings and decorators in 2601: lgtm" — and that puts to rest the earlier discussion about .cleartmask vs. .tmask clear or other single-command structure. Shipping… :shipit:

@dgw dgw merged commit f9a34fb into master Mar 20, 2024
15 checks passed
@dgw dgw deleted the adminchannel-tmask-none branch March 20, 2024 12:54
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) Tweak
Projects
None yet
Development

Successfully merging this pull request may close these issues.

adminchannel: empty .tmask handled incorrectly
3 participants