-
Notifications
You must be signed in to change notification settings - Fork 0
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
IP and email scoring bot #1
base: 7.0.x
Are you sure you want to change the base?
Conversation
…oad in sopel but not on a server.
Co-authored-by: Ross Williams <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall I think this looks good.
There are some areas where my lack of familiarity with sopel hinders this review, and a lot of my points are nitpicks. There's a lot more here than I saw previously, so it makes sense that things might be structured a bit weird for a time while everything is coming together. There aren't any big red flags or issues, and with safe mode as a thing I think the rollback will be pretty smooth.
sopel/modules/ip.py
Outdated
# RPL_YOUREOPER because that functionality is restricted to opers. | ||
rand = str(randint(0, 999)) | ||
while rand in who_reqs: | ||
rand = str(randint(0, 999)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably better as a helper function, generate_request_id()
or something
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copied from other sopel code. Will fix eventually
sopel/modules/ip.py
Outdated
if trigger.is_privmsg and ( trigger.account is None or trigger.account.lower() != "kufat" ): | ||
return | ||
full = ( ( trigger.sender.lower() in ("#skipirc-staff", "#kufat") ) or | ||
( trigger.is_privmsg and trigger.account.lower() == "kufat" ) ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should definitely be constants or in configuration files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as above
except socket.gaierror: | ||
return bot.say("[IP/Host Lookup] Unable to resolve IP/Hostname") | ||
else: | ||
return bot.say("[IP/Host Lookup] Unable to resolve IP/Hostname") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This if-elif-else block seems like it would make sense as a helper function? resolve_ip_info()
or such
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This part is lightly modified stock code, but I agree
Co-authored-by: Ammon Smith <[email protected]>
… remaining review comments.
I had to rush this along when my original prototype IP scoring bot (which was a very, very preliminary version of this) dies because of a WSL hiccup and I was unable to restart it. It has all the basic features I wanted for both IP and email checking, mainly scoring and a persistent database so we don't waste queries (5k/mo quota.)
Haven't tested this yet, although I did make sure it parsed and loaded on a sopel instance that wasn't connected to anything.