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

rewrite of RSS module, two additional modules, other tweaks #230

Closed
wants to merge 6 commits into from
Closed

rewrite of RSS module, two additional modules, other tweaks #230

wants to merge 6 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Apr 16, 2013

Just some stuff I've tweaked this week, feel free to merge if you find these appropriate.

Core

  • Bugfix: socket.read() instead of read()

AI

  • disabled "lol" parsing
  • removed "willie" from regex of all AI methods

Join

  • Joins the bot to the specified channel, eg: .join <#channel>

Raw

  • For sending raw commands to the server, eg: .raw PRIVMSG #lobby :Just testing.

RSS

  • added HTML fom and cookie authentication (depends on mechanize), configurable, work in progress
  • cleaner responses
  • moved everything into one method & reduced footprint
  • removed SUB
  • removed colours
  • removed quotation parsing for site title
  • renamed a bunch of things for readability

@embolalia
Copy link
Contributor

We'd much rather that you limit your pull requests to one focus at a time. It makes it far easier for us to merge that way. To pull this, I'd need to go through and figure out which individual things you've done we actually want. For example, you were right to modify the regexes in ai, but leaving commented code in version-controlled software is generally frowned upon - better to remove it entirely and use the version control to bring it back if you need it. (Not to mention, at the moment I don't think we want to remove that feature.) And we already have .join defined in admin.py, so we don't need to add it again.

All that said, I'm afraid your RSS module introduces a SQL injection vulnerability. Admittedly, you'd have to be an admin to cause it, but such a thing could just as easily lead to a difficult bug down the line. I'm also not sure what RSS authentication is for, or what the purpose is of the mechanize dependency. You also use one command with arguments to do all of the management which, while it isn't necessarily wrong and is in fact used a number of places within the current codebase, is something that I'd like to try to get away from, especially with something as complex as rss.

Regarding RSS specifically, we are planning a new database system which is not yet fully defined. (See issue #194.) The current RSS module needs much more work done than just refactoring to that coming system, so I certainly won't tell you not to redo it before that gets finalized, but when it does come out it'll make some things easier (like, for example, reliably executing more kinds of SQL queries safely).

I don't say all this to dissuade you from contributing; we certainly appreciate it. But following the guidelines in the CONTRIBUTING file will make it much easier to merge your commits.

@ghost ghost closed this Apr 17, 2013
This pull request was closed.
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.

1 participant