Name changer and channel changer

[colwem]

What am I supposed to write in the title and this section.

[taylskid]

If we decided to keep this code, it should probably be

on:message,"hello #{c.nick}"do |m|
m.reply"Hello, #{m.user.nick}"end

Just to keep the channel a little less flooded.

[colwem]

I don't think we should keep this. But if we do you're right. We would also move it into a plugin.

[taylskid]

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

[jfredett]

Apologies for the delay,

Wrt to this pull, the following things are needed:

1. We need to rebase this on master (it's updated since the original pull)

2. The TODO's are better kept as notes on the Pull Request, rather than inline (you can add them as a comment like this one on the PR).

3. the two commits: 212d55f, 8f8bcf5 should be reworded. The latter is unclear as to what you removed, I recommend using the squish and edit commands in the rebase mode, but start by checking out a backup copy of the branch (eg, checkout the name_changer branch, then use git checkout -b name_changer_rebase to keep the name_changer branch as a backup), then when you're done reworking the commits, you can use reset to update the original branch).

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

[travisbot]

This pull request passes (merged 40e14b5 into b33d958).

[travisbot]

This pull request passes (merged e5fdd37 into b33d958).

[jfredett]

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.

[jfredett]

Other than my inline comment about the throw versus the raise, looks good! Once that's squared up, I can merge. Bonus points if you fix the little style things I mentioned, but it's not a big deal if you don't.

Nice job on the rebase by the way. Looks great!

[colwem]

So do I make a new commit for these small changes or should I fit them into
those commits?

On Mon, Aug 13, 2012 at 8:30 PM, Joe Fredette notifications@github.comwrote:

Other than my inline comment about the throw versus the raise, looks
good! Once that's squared up, I can merge. Bonus points if you fix the
little style things I mentioned, but it's not a big deal if you don't.

Nice job on the rebase by the way. Looks great!

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/13#issuecomment-7712858.

[travisbot]

This pull request passes (merged 2711b9f into b33d958).

[colwem]

nevermind I edited the commits and pushed them.

On Mon, Aug 13, 2012 at 9:56 PM, Martin Colwell colwem@gmail.com wrote:

So do I make a new commit for these small changes or should I fit them
into those commits?

On Mon, Aug 13, 2012 at 8:30 PM, Joe Fredette notifications@github.comwrote:

Other than my inline comment about the throw versus the raise, looks
good! Once that's squared up, I can merge. Bonus points if you fix the
little style things I mentioned, but it's not a big deal if you don't.

Nice job on the rebase by the way. Looks great!

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/13#issuecomment-7712858.

[jfredett]

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.

[jfredett]

Approved!

[colwem]

What am I supposed to write in the title and this section.

[taylskid]

If we decided to keep this code, it should probably be

on:message,"hello #{c.nick}"do |m|
m.reply"Hello, #{m.user.nick}"end

Just to keep the channel a little less flooded.

[colwem]

I don't think we should keep this. But if we do you're right. We would also move it into a plugin.

[taylskid]

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

[jfredett]

Apologies for the delay,

Wrt to this pull, the following things are needed:

1. We need to rebase this on master (it's updated since the original pull)

2. The TODO's are better kept as notes on the Pull Request, rather than inline (you can add them as a comment like this one on the PR).

3. the two commits: 212d55f, 8f8bcf5 should be reworded. The latter is unclear as to what you removed, I recommend using the squish and edit commands in the rebase mode, but start by checking out a backup copy of the branch (eg, checkout the name_changer branch, then use git checkout -b name_changer_rebase to keep the name_changer branch as a backup), then when you're done reworking the commits, you can use reset to update the original branch).

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

[travisbot]

This pull request passes (merged 40e14b5 into b33d958).

[travisbot]

This pull request passes (merged e5fdd37 into b33d958).

[jfredett]

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.

[jfredett]

Other than my inline comment about the throw versus the raise, looks good! Once that's squared up, I can merge. Bonus points if you fix the little style things I mentioned, but it's not a big deal if you don't.

Nice job on the rebase by the way. Looks great!

[colwem]

So do I make a new commit for these small changes or should I fit them into
those commits?

On Mon, Aug 13, 2012 at 8:30 PM, Joe Fredette notifications@github.comwrote:

Other than my inline comment about the throw versus the raise, looks
good! Once that's squared up, I can merge. Bonus points if you fix the
little style things I mentioned, but it's not a big deal if you don't.

Nice job on the rebase by the way. Looks great!

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/13#issuecomment-7712858.

[travisbot]

This pull request passes (merged 2711b9f into b33d958).

[colwem]

nevermind I edited the commits and pushed them.

On Mon, Aug 13, 2012 at 9:56 PM, Martin Colwell colwem@gmail.com wrote:

So do I make a new commit for these small changes or should I fit them
into those commits?

On Mon, Aug 13, 2012 at 8:30 PM, Joe Fredette notifications@github.comwrote:

Other than my inline comment about the throw versus the raise, looks
good! Once that's squared up, I can merge. Bonus points if you fix the
little style things I mentioned, but it's not a big deal if you don't.

Nice job on the rebase by the way. Looks great!

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/13#issuecomment-7712858.

[jfredett]

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.

[jfredett]

Approved!

[colwem]

What am I supposed to write in the title and this section.

[taylskid]

If we decided to keep this code, it should probably be

on:message,"hello #{c.nick}"do |m|
m.reply"Hello, #{m.user.nick}"end

Just to keep the channel a little less flooded.

[colwem]

I don't think we should keep this. But if we do you're right. We would also move it into a plugin.

[taylskid]

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

[jfredett]

Apologies for the delay,

Wrt to this pull, the following things are needed:

1. We need to rebase this on master (it's updated since the original pull)

2. The TODO's are better kept as notes on the Pull Request, rather than inline (you can add them as a comment like this one on the PR).

3. the two commits: 212d55f, 8f8bcf5 should be reworded. The latter is unclear as to what you removed, I recommend using the squish and edit commands in the rebase mode, but start by checking out a backup copy of the branch (eg, checkout the name_changer branch, then use git checkout -b name_changer_rebase to keep the name_changer branch as a backup), then when you're done reworking the commits, you can use reset to update the original branch).

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

[travisbot]

This pull request passes (merged 40e14b5 into b33d958).

[travisbot]

This pull request passes (merged e5fdd37 into b33d958).

[jfredett]

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.

[jfredett]

Other than my inline comment about the throw versus the raise, looks good! Once that's squared up, I can merge. Bonus points if you fix the little style things I mentioned, but it's not a big deal if you don't.

Nice job on the rebase by the way. Looks great!

[colwem]

So do I make a new commit for these small changes or should I fit them into
those commits?

On Mon, Aug 13, 2012 at 8:30 PM, Joe Fredette notifications@github.comwrote:

Other than my inline comment about the throw versus the raise, looks
good! Once that's squared up, I can merge. Bonus points if you fix the
little style things I mentioned, but it's not a big deal if you don't.

Nice job on the rebase by the way. Looks great!

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/13#issuecomment-7712858.

[travisbot]

This pull request passes (merged 2711b9f into b33d958).

[colwem]

nevermind I edited the commits and pushed them.

On Mon, Aug 13, 2012 at 9:56 PM, Martin Colwell colwem@gmail.com wrote:

So do I make a new commit for these small changes or should I fit them
into those commits?

On Mon, Aug 13, 2012 at 8:30 PM, Joe Fredette notifications@github.comwrote:

Other than my inline comment about the throw versus the raise, looks
good! Once that's squared up, I can merge. Bonus points if you fix the
little style things I mentioned, but it's not a big deal if you don't.

Nice job on the rebase by the way. Looks great!

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/13#issuecomment-7712858.

[jfredett]

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.

[jfredett]

Approved!

[colwem]

What am I supposed to write in the title and this section.

[taylskid]

If we decided to keep this code, it should probably be

on:message,"hello #{c.nick}"do |m|
m.reply"Hello, #{m.user.nick}"end

Just to keep the channel a little less flooded.

[colwem]

I don't think we should keep this. But if we do you're right. We would also move it into a plugin.

[taylskid]

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

[jfredett]

Apologies for the delay,

Wrt to this pull, the following things are needed:

1. We need to rebase this on master (it's updated since the original pull)

2. The TODO's are better kept as notes on the Pull Request, rather than inline (you can add them as a comment like this one on the PR).

3. the two commits: 212d55f, 8f8bcf5 should be reworded. The latter is unclear as to what you removed, I recommend using the squish and edit commands in the rebase mode, but start by checking out a backup copy of the branch (eg, checkout the name_changer branch, then use git checkout -b name_changer_rebase to keep the name_changer branch as a backup), then when you're done reworking the commits, you can use reset to update the original branch).

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

[travisbot]

This pull request passes (merged 40e14b5 into b33d958).

[travisbot]

This pull request passes (merged e5fdd37 into b33d958).

[jfredett]

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.

[jfredett]

Other than my inline comment about the throw versus the raise, looks good! Once that's squared up, I can merge. Bonus points if you fix the little style things I mentioned, but it's not a big deal if you don't.

Nice job on the rebase by the way. Looks great!

[colwem]

So do I make a new commit for these small changes or should I fit them into
those commits?

On Mon, Aug 13, 2012 at 8:30 PM, Joe Fredette notifications@github.comwrote:

Other than my inline comment about the throw versus the raise, looks
good! Once that's squared up, I can merge. Bonus points if you fix the
little style things I mentioned, but it's not a big deal if you don't.

Nice job on the rebase by the way. Looks great!

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/13#issuecomment-7712858.

[travisbot]

This pull request passes (merged 2711b9f into b33d958).

[colwem]

nevermind I edited the commits and pushed them.

On Mon, Aug 13, 2012 at 9:56 PM, Martin Colwell colwem@gmail.com wrote:

So do I make a new commit for these small changes or should I fit them
into those commits?

On Mon, Aug 13, 2012 at 8:30 PM, Joe Fredette notifications@github.comwrote:

Other than my inline comment about the throw versus the raise, looks
good! Once that's squared up, I can merge. Bonus points if you fix the
little style things I mentioned, but it's not a big deal if you don't.

Nice job on the rebase by the way. Looks great!

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/13#issuecomment-7712858.

[jfredett]

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.

[jfredett]

Approved!

[colwem]

What am I supposed to write in the title and this section.

[taylskid]

If we decided to keep this code, it should probably be

on:message,"hello #{c.nick}"do |m|
m.reply"Hello, #{m.user.nick}"end

Just to keep the channel a little less flooded.

[colwem]

I don't think we should keep this. But if we do you're right. We would also move it into a plugin.

[taylskid]

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

[jfredett]

Apologies for the delay,

Wrt to this pull, the following things are needed:

1. We need to rebase this on master (it's updated since the original pull)

2. The TODO's are better kept as notes on the Pull Request, rather than inline (you can add them as a comment like this one on the PR).

3. the two commits: 212d55f, 8f8bcf5 should be reworded. The latter is unclear as to what you removed, I recommend using the squish and edit commands in the rebase mode, but start by checking out a backup copy of the branch (eg, checkout the name_changer branch, then use git checkout -b name_changer_rebase to keep the name_changer branch as a backup), then when you're done reworking the commits, you can use reset to update the original branch).

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

[travisbot]

This pull request passes (merged 40e14b5 into b33d958).

[travisbot]

This pull request passes (merged e5fdd37 into b33d958).

[jfredett]

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.

[jfredett]

Other than my inline comment about the throw versus the raise, looks good! Once that's squared up, I can merge. Bonus points if you fix the little style things I mentioned, but it's not a big deal if you don't.

Nice job on the rebase by the way. Looks great!

[colwem]

So do I make a new commit for these small changes or should I fit them into
those commits?

On Mon, Aug 13, 2012 at 8:30 PM, Joe Fredette notifications@github.comwrote:

Other than my inline comment about the throw versus the raise, looks
good! Once that's squared up, I can merge. Bonus points if you fix the
little style things I mentioned, but it's not a big deal if you don't.

Nice job on the rebase by the way. Looks great!

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/13#issuecomment-7712858.

[travisbot]

This pull request passes (merged 2711b9f into b33d958).

[colwem]

nevermind I edited the commits and pushed them.

On Mon, Aug 13, 2012 at 9:56 PM, Martin Colwell colwem@gmail.com wrote:

So do I make a new commit for these small changes or should I fit them
into those commits?

On Mon, Aug 13, 2012 at 8:30 PM, Joe Fredette notifications@github.comwrote:

Other than my inline comment about the throw versus the raise, looks
good! Once that's squared up, I can merge. Bonus points if you fix the
little style things I mentioned, but it's not a big deal if you don't.

Nice job on the rebase by the way. Looks great!

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/13#issuecomment-7712858.

[jfredett]

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.

[jfredett]

Approved!

[colwem]

What am I supposed to write in the title and this section.

[taylskid]

If we decided to keep this code, it should probably be

on:message,"hello #{c.nick}"do |m|
m.reply"Hello, #{m.user.nick}"end

Just to keep the channel a little less flooded.

[colwem]

I don't think we should keep this. But if we do you're right. We would also move it into a plugin.

[taylskid]

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

[jfredett]

Apologies for the delay,

Wrt to this pull, the following things are needed:

1. We need to rebase this on master (it's updated since the original pull)

2. The TODO's are better kept as notes on the Pull Request, rather than inline (you can add them as a comment like this one on the PR).

3. the two commits: 212d55f, 8f8bcf5 should be reworded. The latter is unclear as to what you removed, I recommend using the squish and edit commands in the rebase mode, but start by checking out a backup copy of the branch (eg, checkout the name_changer branch, then use git checkout -b name_changer_rebase to keep the name_changer branch as a backup), then when you're done reworking the commits, you can use reset to update the original branch).

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

[travisbot]

This pull request passes (merged 40e14b5 into b33d958).

[travisbot]

This pull request passes (merged e5fdd37 into b33d958).

[jfredett]

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.

[jfredett]

Other than my inline comment about the throw versus the raise, looks good! Once that's squared up, I can merge. Bonus points if you fix the little style things I mentioned, but it's not a big deal if you don't.

Nice job on the rebase by the way. Looks great!

[colwem]

So do I make a new commit for these small changes or should I fit them into
those commits?

On Mon, Aug 13, 2012 at 8:30 PM, Joe Fredette notifications@github.comwrote:

Other than my inline comment about the throw versus the raise, looks
good! Once that's squared up, I can merge. Bonus points if you fix the
little style things I mentioned, but it's not a big deal if you don't.

Nice job on the rebase by the way. Looks great!

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/13#issuecomment-7712858.

[travisbot]

This pull request passes (merged 2711b9f into b33d958).

[colwem]

nevermind I edited the commits and pushed them.

On Mon, Aug 13, 2012 at 9:56 PM, Martin Colwell colwem@gmail.com wrote:

So do I make a new commit for these small changes or should I fit them
into those commits?

On Mon, Aug 13, 2012 at 8:30 PM, Joe Fredette notifications@github.comwrote:

Other than my inline comment about the throw versus the raise, looks
good! Once that's squared up, I can merge. Bonus points if you fix the
little style things I mentioned, but it's not a big deal if you don't.

Nice job on the rebase by the way. Looks great!

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/13#issuecomment-7712858.

[jfredett]

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.

[jfredett]

Approved!

[colwem]

What am I supposed to write in the title and this section.

[taylskid]

If we decided to keep this code, it should probably be

on:message,"hello #{c.nick}"do |m|
m.reply"Hello, #{m.user.nick}"end

Just to keep the channel a little less flooded.

[colwem]

I don't think we should keep this. But if we do you're right. We would also move it into a plugin.

[taylskid]

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

[jfredett]

Apologies for the delay,

Wrt to this pull, the following things are needed:

1. We need to rebase this on master (it's updated since the original pull)

2. The TODO's are better kept as notes on the Pull Request, rather than inline (you can add them as a comment like this one on the PR).

3. the two commits: 212d55f, 8f8bcf5 should be reworded. The latter is unclear as to what you removed, I recommend using the squish and edit commands in the rebase mode, but start by checking out a backup copy of the branch (eg, checkout the name_changer branch, then use git checkout -b name_changer_rebase to keep the name_changer branch as a backup), then when you're done reworking the commits, you can use reset to update the original branch).

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

[travisbot]

This pull request passes (merged 40e14b5 into b33d958).

[travisbot]

This pull request passes (merged e5fdd37 into b33d958).

[jfredett]

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.

[jfredett]

Other than my inline comment about the throw versus the raise, looks good! Once that's squared up, I can merge. Bonus points if you fix the little style things I mentioned, but it's not a big deal if you don't.

Nice job on the rebase by the way. Looks great!

[colwem]

So do I make a new commit for these small changes or should I fit them into
those commits?

On Mon, Aug 13, 2012 at 8:30 PM, Joe Fredette notifications@github.comwrote:

Other than my inline comment about the throw versus the raise, looks
good! Once that's squared up, I can merge. Bonus points if you fix the
little style things I mentioned, but it's not a big deal if you don't.

Nice job on the rebase by the way. Looks great!

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/13#issuecomment-7712858.

[travisbot]

This pull request passes (merged 2711b9f into b33d958).

[colwem]

nevermind I edited the commits and pushed them.

On Mon, Aug 13, 2012 at 9:56 PM, Martin Colwell colwem@gmail.com wrote:

So do I make a new commit for these small changes or should I fit them
into those commits?

On Mon, Aug 13, 2012 at 8:30 PM, Joe Fredette notifications@github.comwrote:

Other than my inline comment about the throw versus the raise, looks
good! Once that's squared up, I can merge. Bonus points if you fix the
little style things I mentioned, but it's not a big deal if you don't.

Nice job on the rebase by the way. Looks great!

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/13#issuecomment-7712858.

[jfredett]

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.

[jfredett]

Approved!

[colwem]

What am I supposed to write in the title and this section.

[taylskid]

If we decided to keep this code, it should probably be

on:message,"hello #{c.nick}"do |m|
m.reply"Hello, #{m.user.nick}"end

Just to keep the channel a little less flooded.

[colwem]

I don't think we should keep this. But if we do you're right. We would also move it into a plugin.

[taylskid]

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

[jfredett]

Apologies for the delay,

Wrt to this pull, the following things are needed:

1. We need to rebase this on master (it's updated since the original pull)

2. The TODO's are better kept as notes on the Pull Request, rather than inline (you can add them as a comment like this one on the PR).

3. the two commits: 212d55f, 8f8bcf5 should be reworded. The latter is unclear as to what you removed, I recommend using the squish and edit commands in the rebase mode, but start by checking out a backup copy of the branch (eg, checkout the name_changer branch, then use git checkout -b name_changer_rebase to keep the name_changer branch as a backup), then when you're done reworking the commits, you can use reset to update the original branch).

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

[travisbot]

This pull request passes (merged 40e14b5 into b33d958).

[travisbot]

This pull request passes (merged e5fdd37 into b33d958).

[jfredett]

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.

[jfredett]

Other than my inline comment about the throw versus the raise, looks good! Once that's squared up, I can merge. Bonus points if you fix the little style things I mentioned, but it's not a big deal if you don't.

Nice job on the rebase by the way. Looks great!

[colwem]

So do I make a new commit for these small changes or should I fit them into
those commits?

On Mon, Aug 13, 2012 at 8:30 PM, Joe Fredette notifications@github.comwrote:

Other than my inline comment about the throw versus the raise, looks
good! Once that's squared up, I can merge. Bonus points if you fix the
little style things I mentioned, but it's not a big deal if you don't.

Nice job on the rebase by the way. Looks great!

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/13#issuecomment-7712858.

[travisbot]

This pull request passes (merged 2711b9f into b33d958).

[colwem]

nevermind I edited the commits and pushed them.

On Mon, Aug 13, 2012 at 9:56 PM, Martin Colwell colwem@gmail.com wrote:

So do I make a new commit for these small changes or should I fit them
into those commits?

On Mon, Aug 13, 2012 at 8:30 PM, Joe Fredette notifications@github.comwrote:

Other than my inline comment about the throw versus the raise, looks
good! Once that's squared up, I can merge. Bonus points if you fix the
little style things I mentioned, but it's not a big deal if you don't.

Nice job on the rebase by the way. Looks great!

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/13#issuecomment-7712858.

[jfredett]

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.

[jfredett]

Approved!