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

admin: Catch set commands without arguments #1520

Merged
merged 1 commit into from
Apr 2, 2019
Merged

Conversation

kwaaak
Copy link
Contributor

@kwaaak kwaaak commented Mar 25, 2019

Before:
.set
AttributeError: 'NoneType' object has no attribute 'split'

After:
.set
Owner: Usage: .set section.option value

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.

Nice catch!

I made a comment for 2 reasons:

  • naming things (arg1 is not very expressive) edit: it was arg1 before, so let's keep it for now!
  • readability is more important than saving one line, return should be on its own

sopel/modules/admin.py Outdated Show resolved Hide resolved
@Exirel Exirel added this to the 6.6.6 milestone Mar 26, 2019
@dgw dgw added Bugfix Generally, PRs that reference (and fix) one or more issue(s) and removed Tweak labels Mar 26, 2019
@dgw
Copy link
Member

dgw commented Mar 26, 2019

@Exirel I would like to point out that arg1 was the preexisting variable name, so let's not change it in the whole command in this PR.

@Exirel
Copy link
Contributor

Exirel commented Mar 26, 2019

arg1 was the preexisting variable name

Fair enough. Let's keep it that way for now then!

@kwaaak
Copy link
Contributor Author

kwaaak commented Mar 26, 2019

Philosophical question whether

return bot.say()

or

bot.say()
return

is more readable 💥

Changed it to the latter, to keep the style consistent with the rest of the file.

@Exirel
Copy link
Contributor

Exirel commented Mar 26, 2019

is more readable boom

As I said on IRC, I think it's not about readability, but about intent and being explicit. My reasons is that return bot.say(...) asks these questions:

  • does bot.say return something?
  • does it matter?
  • why a command return something, albeit being run in a separated thread?
  • what's the meaning of returning something?

which is quite confusing. As pointed out by Pilate on IRC (PEP 0020):

Explicit is better than implicit

and in that regard, an empty return statement is, by convention, the explicit way to tell "stop and quit". There is extra question, no confusion possible.

That's my opinion, at least.

Copy link
Member

@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.

Straightforward, this is.

@dgw dgw requested a review from Exirel March 26, 2019 20:32
Before:
<Owner> .set
<Sopel> AttributeError: 'NoneType' object has no attribute 'split'

After:
<Owner> .set
<Sopel> Owner: Usage: .set section.option value
@dgw dgw merged commit 31343ec into sopel-irc:6.6.x Apr 2, 2019
@kwaaak kwaaak deleted the patch-1 branch May 20, 2019 22:02
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) Low Priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants