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

Reorganise whois.set functionality #130

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

bensimner
Copy link
Contributor

Alias !whois.set to !whois.setdefault and add !whois.setlocal.
Also alias !whois.unset to !whois.unsetdefault and add !whois.unsetlocal

This PR also adds useful !help text for the commands in the whois plugin.

Alias !whois.set to !whois.setdefault and add !whois.setlocal.
Also alias !whois.unset to !whois.unsetdefault and add !whois.unsetlocal
Pedantic change to be consistent.
@coveralls
Copy link

coveralls commented Jun 30, 2018

Coverage Status

Coverage increased (+0.1%) to 70.245% when pulling 6ebb66a on bensimner:whois-setdefault-default into 9774225 on HackSoc:master.

csbot/plugins/whois.py Outdated Show resolved Hide resolved
csbot/plugins/whois.py Outdated Show resolved Hide resolved
@LordAro
Copy link
Member

LordAro commented Jun 30, 2018

Related #66

This change makes !set always succeed in setting the whois for this channel, but it will now never over-write the global value if one exists.
It also means that !unset behaves similarly, it always succeeds in unsetting whichever whois was set for the channel, including removing the global one should it exist.
I think more work needs to be done to decide whether that last decision is a good one: removing/setting global data on accident is bad UX but it's better than what we currently have, and better than unexpectedly being unable to set the whois.
Copy link
Member

@LordAro LordAro left a comment

Choose a reason for hiding this comment

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

I guess I'm ok with this, though I think it could be confusing...

@LordAro
Copy link
Member

LordAro commented Sep 23, 2018

Needs a small amount of rewriting for new test style

@LordAro
Copy link
Member

LordAro commented Jan 27, 2019

@bensimner branch needs a rebase (and probably a further one after #142 is merged)

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

Successfully merging this pull request may close these issues.

3 participants