531785 - [Patch] Get rid of leading ampersand in subroutine calls

Status: RESOLVED FIXED

(Webtools Graveyard :: Mozbot, defect)

Description

[Shlomi Fish]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2b4) Gecko/20091128 Mandriva Linux/1.9.2-0.b4.1mdv2010.1 (2010.1) Firefox/3.6b4
Build Identifier: 

Hi all!

I'm in the process of cleaning up and modernising the Mozbot code and noticed that it uses many &mysubroutine(@args) function calls, which is a sign of Ancient Perl:

http://www.perlfoundation.org/perl5/index.cgi?subroutines_called_with_the_ampersand

As a result, I wrote this patch to get rid of all of them that I could find using a search:

http://www.shlomifish.org/Files/files/code/mozbot/mozbot-get-rid-of-amp-func-call.diff

I had to rename a few functions which clashed with Perl built-ins or keywords.

Regards,

-- Shlomi Fish

Reproducible: Always

Comment 1

[Shlomi Fish]

Attachment: The Patch to fix the ampersand-subroutine problem

Here is the patch in the URL.

Comment 2

[Shlomi Fish]

I should note that I continued to work on cleaning up Mozbot here:

http://bitbucket.org/shlomif/mozbot-shlomif/

Regards,

-- Shlomi Fish

Comment 3

[Tanner M. Young [:tmyoung]]

Comment on attachment 415094 [details] [diff] [review]
The Patch to fix the ampersand-subroutine problem

Requesting review from Wolf.

Comment 4

[Wolf [:wolf]]

Comment on attachment 415094 [details] [diff] [review]
The Patch to fix the ampersand-subroutine problem

Is this patch current? (Bug comments imply there may be newer work...)
Has it been tested?

Comment 5

[Shlomi Fish]

(In reply to comment #4)
> (From update of attachment 415094 [details] [diff] [review])
> Is this patch current? (Bug comments imply there may be newer work...)

Well, there are further cleanups in the Mercurial repository (which can be applied later).

> Has it been tested?

I tested it by running the script from the command line and seeing if it emits anything. Didn't really try to really operate it and see if it behaves.

Comment 6

[Tanner M. Young [:tmyoung]]

I'll do a checkout of the hg repository and test it alongside the other bots that I have.

Comment 7

[Tanner M. Young [:tmyoung]]

I just ran it, and I got the following after connecting to the server

Uncaught exception from user code:
     No method called "_local_all_modules" for object. at ./mozbot.pl line 701

I goes on to give a full trace through IRC::Connection, but I have to type these errors manually since I am using a Cygwin SSL client, which has no copy ability.  If you need the full stack I can provide it for you, when I have a little more time.

Comment 8

[Tanner M. Young [:tmyoung]]

My mistake...It should read:

     No method called "_load_all_modules" for object. at ./mozbot.pl line 701

Comment 9

[Shlomi Fish]

(In reply to comment #8)
> My mistake...It should read:
> 
>      No method called "_load_all_modules" for object. at ./mozbot.pl line 701

Hi!

Sorry for the late response. Thanks for the report - I'll try to investigate. I should note that you can use a decent console implementation for cygwin with decent copy and paste capabilities such as the various X-based terminal windows (xterm, rxvt, gnome-terminal, konsole, etc.) or alternatively:

* http://sourceforge.net/projects/console/

Regards,

-- Shlomi Fish

Comment 10

[Shlomi Fish]

OK, I was able to reproduce this problem. Please see the latest changes on http://bitbucket.org/shlomif/mozbot-shlomif . Now "mozbot-rindolf" connects to the channel but does not appear to be responsive at all because of some reason.

Comment 11

[Tanner M. Young [:tmyoung]]

Okay, I'll keep testing and mentioning anything I find in here.  When you make a change that you would like tested, just tell me in the bug.

Thank you!

Comment 12

[Shlomi Fish]

(In reply to comment #11)
> Okay, I'll keep testing and mentioning anything I find in here.  When you make
> a change that you would like tested, just tell me in the bug.
> 

OK, please test the version that I have now in the Mercurial repository.

Regards,

-- Shlomi Fish

> Thank you!

Comment 13

[Tanner M. Young [:tmyoung]]

I have updated to your current tip; the bot loads, connects to the server, and joins channels, but it doesn't respond to any commands.

I tried auth admin password, hi, ping, and help--all of which should work out of the box.  It shows both Told and Baffled entries in the log for the commands I sent, but it doesn't have any other log data.

If you need a server to connect to for testing, just connect to irc.mozilla.org and join #mozbot.  I'll be around there if you need me.

Comment 14

[Shlomi Fish]

Hi,

sorry it took me so long.

(In reply to comment #13)
> I have updated to your current tip; the bot loads, connects to the server, and
> joins channels, but it doesn't respond to any commands.
> 
> I tried auth admin password, hi, ping, and help--all of which should work out
> of the box.  It shows both Told and Baffled entries in the log for the commands
> I sent, but it doesn't have any other log data.
> 
> If you need a server to connect to for testing, just connect to irc.mozilla.org
> and join #mozbot.  I'll be around there if you need me.

OK, now I've tested my current tip of:

http://bitbucket.org/shlomif/mozbot-shlomif/

It connects to the channel fine and handles the basic commands of "hi", "remember" and "help".

Please test it.

Regards,

-- Shlomi Fish

Comment 15

[Tanner M. Young [:tmyoung]]

Everything I tried works except "auth admin password" or "auth admin password quiet"  The bot doesn't return any data on this command and there is nothing in the logs.  Otherwise, it is looking very good and it appears to be faster :)

Comment 16

[Shlomi Fish]

(In reply to comment #15)
> Everything I tried works except "auth admin password" or "auth admin password
> quiet"  The bot doesn't return any data on this command and there is nothing in
> the logs.  Otherwise, it is looking very good and it appears to be faster :)

Thanks, I'll investigate that and return to you.

Regards,

-- Shlomi Fish

Comment 17

[Shlomi Fish]

Hi Tanner,

(In reply to comment #16)
> (In reply to comment #15)
> > Everything I tried works except "auth admin password" or "auth admin password
> > quiet"  The bot doesn't return any data on this command and there is nothing in
> > the logs.  Otherwise, it is looking very good and it appears to be faster :)
> 
> Thanks, I'll investigate that and return to you.
> 

I apologise for the late response, but I only got to it now. After some investigation, I've fixed the code now by replacing the call to not_in_channel() with a call to in_channel(), and this appears to have fixed the functionality.

Please review again.

Regards,

-- Shlomi Fish

Comment 18

[Tanner M. Young [:tmyoung]]

It looks good to me; I didn't find anything else wrong.  If you could post the changes as a new patch to this bug, then Wolf can do a final review.

I may run more tests using code coverage (if I have the time) to hopefully ensure there were no commands I missed.

Comment 19

[Shlomi Fish]

Attachment: Mozbot Cleanups Patch

This is the patch as generated by hg push and "cvs diff -u". Please review it, and if it's good - apply it.

Comment 20

[Shlomi Fish]

(In reply to comment #18)
> It looks good to me; I didn't find anything else wrong.  If you could post the
> changes as a new patch to this bug, then Wolf can do a final review.

Done. See attachment 498287 [details] [diff] [review] in the previous comment.

> 
> I may run more tests using code coverage (if I have the time) to hopefully
> ensure there were no commands I missed.

Thanks!

Regards,

-- Shlomi Fish

Comment 21

[Reed Loden [:reed]]

Comment on attachment 498287 [details] [diff] [review]
Mozbot Cleanups Patch

@@ -306,17 +306,17 @@
         # it here.
         return $event->args(\@_);
     } else {
-        return $event->args(@_);
+        return $event->args(\@_);
     }
 }
 
 Same thing now... combine those?


+                # Note from Shlomi Fish:
+                # There was a missing semicolon in the above statement and due
+                # to the fact debug(...) was written &debug instead of debug
+                # it probably yielded a bitwise AND-operation:
+                # x & y.

Just fix the bug. No need for the comment.


+    # tell the modules they have joined IRC

No need for comment.


+        ebug("Trying again with our real username, hold on.");

debug()


Why have a save_config_vars() and not a matching get_config_var()?


Need a better name than 'mydo'.


+    my %dispatch_by_type = 
+    (
+        (map { $_ => sub { return $self->$type($e, $e->{'from'}, $e->{'data'});}, } qw(CTCPPing CTCPSource CTCPVersion SpottedNickChange SpottedQuit)),
+    (map { $_ => sub { return $self->$type($e, $e->{'channel'}, $e->{'data'}, $e->{'from'});}, } qw(ModeChange)),
+(map { $_ => sub { return $self->$type($e, $e->{'data'});}, } qw(Baffled Felt Heard Invited Noticed Saw Told)),
+    (map { $_ => sub { return $self->$type($e, $e->{'channel'}, $e->{'data'});}, } qw(SpottedKick SpottedTopicChange)),
+(map { $_ => sub { return $self->$type($e, $e->{'channel'});}, } qw(Kicked)),
+    (map { $_ => sub { return $self->$type($e, $e->{'from'});}, } qw(Authed)),
+(map { $_ => sub { return $self->$type($e, $e->{'channel'}, $e->{'from'});}, } qw(GotDeopped GotOpped SpottedDeopping SpottedJoin SpottedOpping SpottedPart)),
+    );

This needs cleaning-up. I can barely read it.


 sub saveConfig {
     my $self = shift;
-    &Configuration::Save($cfgfile, $self->configStructure());
+    Configuration::Save($cfgfile, $self->configStructure());
 }
 
 Why not save_config_vars() here? :)


 +use Mozbot::Constants;

Honestly, these separate packages should be split out of mozbot.pl into separate files... I was surprised by your use of 'use' statements in what I thought was the middle of a file's scope (until I pulled more context in to see). You don't have to split this out yourself, but it's just something to think about.


_DefaultGet() seems a bit pointless...


+        'auth' => q{Authenticate yourself. Append the word 'quiet' after your password if you don't want confirmation. Syntax: auth <username> <password> [quiet]},

I'm happy with your use of q{}, but you should use it for every option.

...

I stopped reading, as the patch just starts changing a number of things. Definitely huge scope-creep. I'd be much happier if this patch addressed only what this bug states: removing the use of &s.

Comment 22

[Wolf [:wolf]]

Comment on attachment 498287 [details] [diff] [review]
Mozbot Cleanups Patch

Just for the record, I asked reed to take a look at this patch for his comments, since a patch this large can benefit from extra eyes.

This patch has been bitrotted slightly by the landing of bug 490052 (which modifies lines 1115-1148. The patch appears to be missing the file where Mozbot::Constants is added, BTW.

I'm all for removing the & for ancient perl. Though this patch contains significant refactoring in addition to that. (Which is not - automatically - bad, though it does make it hard to review.)

That said, this needs to be broken up into parts. The basic & removal and the changes required for that. (The original scope of this bug) The refactoring of the Admin module and the Base module should be handled separately. That way the reasoning behind the changes can be made clear and regressions easier to work through. There's alot of changes there that i'm not sure I completely understand the goal of.

One error I hit with this patch (from the Admin module refactoring, I'm guessing):
2011-01-16 08:30:53 UTC (3548) Module Admin: Noticed that source code of ./mozbot.pl had changed
2011-01-16 08:30:53 UTC (3548) ERROR!!!
2011-01-16 08:30:53 UTC (3548) Can't call method "quit" on an undefined value at mozbot.pl line 3033.

Comment 23

[Wolf [:wolf]]

Attachment: &Subroutine cleanup (Mozbot Cleanups Part #1)

This is the portion of the larger patch that specifically covers the original bug topic - removing the & sigil from the subroutine calls (and renaming the methods that conflict with perl afterwards.) - updated to current tip.

The refactoring patches can be handled as followups...
(And the comment about breaking out the additional packages that are contained in mozbot.pl i've filed as Bug 626330)

Comment 24

[Wolf [:wolf]]

Comment on attachment 504369 [details] [diff] [review]
&Subroutine cleanup (Mozbot Cleanups Part #1)

This part of the patch is good (and simple enough). Tested it on testbot.

Cancelling review request for reed - as second-review is a waste of time here - for this part anyway.

Comment 25

[Wolf [:wolf]]

Comment on attachment 504369 [details] [diff] [review]
&Subroutine cleanup (Mozbot Cleanups Part #1)

Checking in mozbot.pl;
/cvsroot/mozilla/webtools/mozbot/mozbot.pl,v  <--  mozbot.pl
new revision: 2.66; previous revision: 2.65
done
Checking in Infobot.pl;
/cvsroot/mozilla/webtools/mozbot/BotModules/Infobot.pl,v  <--  Infobot.pl
new revision: 1.3; previous revision: 1.2
done

Comment 26

[Shlomi Fish]

Hi all! 

Sorry for not commenting, but I did not get the emails for Wolf's comments. Which patch was applied? Were all my changes applied? I want more status stuff.

I don't know how to update the CVS checkout I have properly because I'm getting conflicts and cvs update -C (whatever that is) does not work properly. I'm getting:

<QUOTE>

Index: mozbot.pl
===================================================================
RCS file: /cvsroot/mozilla/webtools/mozbot/mozbot.pl,v
retrieving revision 2.66
diff -u -r2.66 mozbot.pl
--- mozbot.pl	18 Jan 2011 04:46:58 -0000	2.66
+++ mozbot.pl	21 Jun 2011 09:51:19 -0000
@@ -270,8 +270,13 @@
 # save configuration straight away, to make sure it is possible and to save
 # any initial settings on the first run, if anything changed.
 if ($changed) {
+<<<<<<< mozbot.pl
+    debug("saving configuration to '$cfgfile'...");
+    save_config_vars([]);
+=======
     debug("saving configuration to '$cfgfile'...");
     Configuration::Save($cfgfile, configStructure());
+>>>>>>> 2.66
 }
 
 } # close the scope for the $changed variable
@@ -307,6 +312,11 @@
         return $event->args(@_);
     } else {
         return $event->args(\@_);
+<<<<<<< mozbot.pl
+    } else {
+        return $event->args(\@_);
+=======
+>>>>>>> 2.66
     }
 }
 
@@ -514,7 +524,11 @@

</QUOTE>

And I don't know what to do with it. Why is Mozbot using CVS, for crying out loud? We're in 2011 - not in 1996.

Can you please enlighten me how to generate a new diff based on my further work? And shouldn't this bug be closed?

Comment 27

[Shlomi Fish]

Hi all,

(In reply to comment #26)
> Hi all! 
> 
> Sorry for not commenting, but I did not get the emails for Wolf's comments.
> Which patch was applied? Were all my changes applied? I want more status
> stuff.
> 

Any news?

Regards,

-- Shlomi Fish

> I don't know how to update the CVS checkout I have properly because I'm
> getting conflicts and cvs update -C (whatever that is) does not work
> properly. I'm getting:
> 
> <QUOTE>
> 
> Index: mozbot.pl
> ===================================================================
> RCS file: /cvsroot/mozilla/webtools/mozbot/mozbot.pl,v
> retrieving revision 2.66
> diff -u -r2.66 mozbot.pl
> --- mozbot.pl	18 Jan 2011 04:46:58 -0000	2.66
> +++ mozbot.pl	21 Jun 2011 09:51:19 -0000
> @@ -270,8 +270,13 @@
>  # save configuration straight away, to make sure it is possible and to save
>  # any initial settings on the first run, if anything changed.
>  if ($changed) {
> +<<<<<<< mozbot.pl
> +    debug("saving configuration to '$cfgfile'...");
> +    save_config_vars([]);
> +=======
>      debug("saving configuration to '$cfgfile'...");
>      Configuration::Save($cfgfile, configStructure());
> +>>>>>>> 2.66
>  }
>  
>  } # close the scope for the $changed variable
> @@ -307,6 +312,11 @@
>          return $event->args(@_);
>      } else {
>          return $event->args(\@_);
> +<<<<<<< mozbot.pl
> +    } else {
> +        return $event->args(\@_);
> +=======
> +>>>>>>> 2.66
>      }
>  }
>  
> @@ -514,7 +524,11 @@
> 
> </QUOTE>
> 
> And I don't know what to do with it. Why is Mozbot using CVS, for crying out
> loud? We're in 2011 - not in 1996.
> 
> Can you please enlighten me how to generate a new diff based on my further
> work? And shouldn't this bug be closed?

Comment 28

[Shlomi Fish]

(In reply to Shlomi Fish from comment #27)
> Hi all,
> 
> (In reply to comment #26)
> > Hi all! 
> > 
> > Sorry for not commenting, but I did not get the emails for Wolf's comments.
> > Which patch was applied? Were all my changes applied? I want more status
> > stuff.
> > 
> 
> Any news?
> 
> Regards,
> 
> -- Shlomi Fish
> 

Ping.

Comment 29

[Tanner M. Young [:tmyoung]]

The second patch (&Subroutine cleanup (Mozbot Cleanups Part #1) (48.21 KB, patch)) was committed.  As far as the question of why CVS wont update, my guess is that since you have the changes you are downloading already present locally so CVS doesn't want to overwrite your local work.  I think your best bet may be to do a clean checkout of Mozbot and try applying your diff or modify your diff by hand, but I'm afraid I know the least about CVS of all of the VCSes: Reed or Wolf will probably be able to help you a lot more than I can.  There may be a command that covers exactly what you need, but I don't know it off hand.

Comment 30

[Wolf [:wolf]]

(In reply to Shlomi Fish from comment #26)
> 
> And I don't know what to do with it. Why is Mozbot using CVS, for crying out
> loud? We're in 2011 - not in 1996.
> 
> Can you please enlighten me how to generate a new diff based on my further
> work? And shouldn't this bug be closed?

Mozbot isn't very active, and I haven't found moving VCSes to be worthwhile - and I'm not an active enough programmer to have learned git or hg, svn is what i'm most comfortable with.

You're conflicting because my patch did not include the save_config_vars() function you created and the second issue is where I collapsed two codepaths that became the same after the changes. Not sure why you'd be hitting that if you're requesting a clean copy though.

As for closing this bug - it probably should be, additional cleanup work beyond the scope of this bug can be done in follow-up bugs. Though I was waiting for your response before doing so.

Comment 31

[Shlomi Fish]

(In reply to Wolf [:wolf] from comment #30)
> (In reply to Shlomi Fish from comment #26)
> > 
> > And I don't know what to do with it. Why is Mozbot using CVS, for crying out
> > loud? We're in 2011 - not in 1996.
> > 
> > Can you please enlighten me how to generate a new diff based on my further
> > work? And shouldn't this bug be closed?
> 
> Mozbot isn't very active, and I haven't found moving VCSes to be worthwhile
> - and I'm not an active enough programmer to have learned git or hg, svn is
> what i'm most comfortable with.

Well, it still interferes with my attempt to contribute to it.

Right now I'm getting:

[SHELL]
shlomif@telaviv1:~/progs/perl/net/irc/bots/mozbot/cvssrc/mozbot$ cvs up
cvs update: warning: failed to open /home/shlomif/.cvspass for reading: No such file or directory
? .hg
? .hgignore
? .hgtags
? mozbot.diff
? lib/Mozbot
cvs update: Updating .
C mozbot.pl
cvs update: Updating BotModules
cvs update: Updating BotModules/Quiz
cvs update: Updating config
cvs update: Updating lib
cvs update: Updating lib/IO
cvs update: Updating uuidgen
[/SHELL]

And cvs diff mozbot.pl generates a large diff, and, for the life of me, I cannot tell how to revert the local changes on CVS (i.e: "svn revert", "git checkout"/"git reset", or "hg revert"). Googling found some advice which didn't work.

> 
> You're conflicting because my patch did not include the save_config_vars()
> function you created and the second issue is where I collapsed two codepaths
> that became the same after the changes. Not sure why you'd be hitting that
> if you're requesting a clean copy though.
> 
> As for closing this bug - it probably should be, additional cleanup work
> beyond the scope of this bug can be done in follow-up bugs. Though I was
> waiting for your response before doing so.

Regards,

-- Shlomi Fish

Comment 32

[Wolf [:wolf]]

-> FIXED.

Please file additional bugs for follow-up cleanup work.