Name changer and channel changer #13
Conversation
|
|
||
| #just to check if he's responding | ||
| on :message, "hello" do |m| | ||
| m.reply "Hello, #{m.user.nick}" |
There was a problem hiding this comment.
If we decided to keep this code, it should probably be
on :message, "hello #{c.nick}" do |m|
m.reply "Hello, #{m.user.nick}"
endJust to keep the channel a little less flooded.
There was a problem hiding this comment.
I don't think we should keep this. But if we do you're right. We would also move it into a plugin.
There was a problem hiding this comment.
It might be fun to have just a few fun commands like that. Things like !nvidia would link to this: http://www.eteknix.com/wp-content/uploads/2012/06/LinuxVsNvidia.png (lol). or !help to the wiki (or site).
|
Apologies for the delay, Wrt to this pull, the following things are needed:
-- Generally, the commits should be 'logical' -- eg, they should group related changes together, and should avoid redundant/unnecessary changes (eg, add debugs, remove debugs, add comments, remove comments, etc) -- Ideally those things don't get commited in the first place, but in a pinch, they can be rebased away. I know those things seem nitpicky, but git history is a powerful tool in long term maintainership of a repo. The code itself seems good. The tests seem to fail for me, but not yours. I chalk that up to my brittle tests and not your stuff. As for rebasing/reworking, it's really just those bottom three commits (starting at 09031da) that seem to be real offenders. After that stuff gets fixed and the pull is updated, I'll merge this. @SirSkidmore 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. |
|
|
||
| match /change-name\s+(\S+)/, :method => :change_name | ||
|
|
||
| def change_name irc, name |
There was a problem hiding this comment.
The standard style for methods is def method(argument, arg2, arg3), including the parens. In the interest of getting this merged, I'm going to fix this in a different commit.
|
Other than my inline comment about the Nice job on the rebase by the way. Looks great! |
|
So do I make a new commit for these small changes or should I fit them into On Mon, Aug 13, 2012 at 8:30 PM, Joe Fredette notifications@github.comwrote:
|
|
nevermind I edited the commits and pushed them. On Mon, Aug 13, 2012 at 9:56 PM, Martin Colwell colwem@gmail.com wrote:
|
| name_changer: @test_users} | ||
|
|
||
| def self.approved?(user, role) | ||
| user = user.name if user.is_a? Cinch::User |
There was a problem hiding this comment.
On thing to note here (not a major issue)
It's good practice to ask what something can do rather than what something is. For instance, if I were to build a proxy object for Cinch::User which provided some extra methods or something, it might not .is_a? the way you think, but it still can fulfill the contract of this method (since it just proxies down to Cinch::User#name, perhaps).
A concrete example might be a multi-name tracker, we might have a model Cinch::MultiUser, which tracks a user who uses multiple names that don't fit some schema, but we want to grant approval to all of them. We might also want to have a Cinch::AuthenticatedUser, which represents a Freenode auth'd user. etc.
Just something to keep in mind.
|
|
Approved!
Name changer and channel changer
What am I supposed to write in the title and this section.