Closed Bug 191863 Opened 21 years ago Closed 21 years ago

Clean up Bugzilla.pm

Categories

(Bugzilla :: Bugzilla-General, defect)

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Bugzilla 2.18

People

(Reporter: bbaetz, Assigned: bbaetz)

Details

Attachments

(1 file)

As discussed recently on developers and IRC, I've tidied up Bugzilla.pm a bit.

Nothing fancy. Note that the Bugzilla->* methods are still object-methods,
rather than subs. Theres no harm in this, and I have this vague idea to allow
'inheritance' of this at some point (via AUTOLOAD, similar to how File::Spec
works) for mod_perl-ish stuff. It also makes the plugin code a bit nicer, since
Template::Plugin::Procedural isn't in a stable TT release.
Attached patch patchSplinter Review
Attachment #113485 - Flags: review?(justdave)
Comment on attachment 113485 [details] [diff] [review]
patch

OK, this looks good.  I haven't tried it, but it looks pretty straightforward.

I'd feel a little better if someone could do a more thorough review (i.e. try
it) but I'm too wiped out right now to do that.
Attachment #113485 - Flags: review?(justdave)
Attachment #113485 - Flags: review?
Attachment #113485 - Flags: review+
Comment on attachment 113485 [details] [diff] [review]
patch

Gerv?
Attachment #113485 - Flags: review? → review?(gerv)
Comment on attachment 113485 [details] [diff] [review]
patch

Gerv?
Comment on attachment 113485 [details] [diff] [review]
patch

Gerv?
Comment on attachment 113485 [details] [diff] [review]
patch

>+Note that all C<Bugzilla> functionailty is method based; use C<Bugzilla->dbh>
>+rather than C<Bugzilla::DBH>. Nothing cares about this now, but don't rely on
>+that.

I don't quite understand this comment. Perhaps you could clarify it.

>Index: processmail
>===================================================================
>RCS file: /cvsroot/mozilla/webtools/bugzilla/processmail,v
>retrieving revision 1.94
>diff -u -r1.94 processmail
>--- processmail	29 Dec 2002 07:06:41 -0000	1.94
>+++ processmail	4 Feb 2003 12:23:55 -0000
>@@ -28,6 +28,10 @@
> use strict;
> use lib ".";
> 
>+use Bugzilla;
>+
>+Bugzilla->init({ TYPE => Bugzilla::TYPE_CMDLINE});

How does this work? I don't remember seeing this value looked at anywhere. Or
is it not in this patch (i.e. already present)?

Assuming you've caught all the relevant call sites, then r=gerv.

Gerv
Attachment #113485 - Flags: review?(gerv)
I'm not sure how much clearer I can make it. (Apart from me having the case
wrong in the second example, that is - fixed locally)

What I wanted to say was that to get the current dbh, do:

my $dbh = Bugzilla->dbh();

not

my $dbh = Bugzilla::dbh();

nothing will notice if you use ::dbh ATM, but they may one day. They may not,
but its better to be consistant anyway. (They will notice when there are
arguments passed, obviously, but none of those subs take arguments) This isn't
likely to be a major issue, since people are likly to be copying and pasting the
code anyway.

except that that is what I said... Do you have a better way to say it? :)

Ignore the processmail hunk - that was left over from a previous version of the
patch. I did grep for Bugzilla, and "->instance", so I think I got everything.

Let me know what you want me to change, or I'll check this in when justdave
approves it.
Status: NEW → ASSIGNED
Flags: approval?
Target Milestone: --- → Bugzilla 2.18
Flags: approval? → approval+
Fixed
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: