search

LearnProgramming / percival

An IRC bot designed to help the LPMCers track quotes, log channels, and all sorts of things
22 stars 8 forks source link

Server Configuration Variable #17

Closed taylskid closed 12 years ago

taylskid commented 12 years ago

Allows server to be set in execution OR default to irc.freenode.com

jfredett commented 12 years ago

Since you're pull is included in this one, I'm going to just merge this one (since it makes my job easier), Will also post this on your PR before closing it.

-- I will note that there are a lot of commits here for what should be a very small feature (2 or 3 would be better, though I'd probably do this all in one.) On a lot of FOSS projects, you'll get rejected simply because there's too much in the commit.

For this, I'd rebase to squish all the README commits together, and delete/squish the tabbing issue away as well. Keep the forked by commit (ab1dd81) separate, followed by the README commits (squished together), then the "Remove usless :hello", then squish your implementation to one commit. That clearly shows me (the maintainer) the intent of each commit.

Also note that it is idiomatic git usage to write commit messages in the present tense, rather than "Added a default..." it would be "Add default ...", etc.

You don't need to do this for this pull, since it will not be merged (since the functionality is included in @colwem's PR, but in the future that's some good things to keep in mind.