gisle / tcl.pm

Tcl extension module for Perl
http://search.cpan.org/dist/Tcl
9 stars 8 forks source link

Add and update strictures #6

Closed paultcochrane closed 8 years ago

paultcochrane commented 8 years ago

Add warnings and strict pragmas to files missing these. Also converted the -w flag to the perl binary in the shebang line to warnings pragmas as necessary, since this is currently accepted best practice.

This pull request is submitted in the hope that it will be useful. Any questions and/or comments are most welcome!

vadrer commented 8 years ago

thanks for all these contributions, we'll accept them. "use warnings;" in Tcl.pm is not likely, though, the module sometimes could be used without anything at all, therefore Tcl.pm is made to be especially light and not module-dependant.

Most changes will be applied though, thanks for the contribution :)

paultcochrane commented 8 years ago

Only too pleased to help out! Is there anything in particular you would like me to focus on?

paultcochrane commented 8 years ago

As an alternative to having use warnings set everywhere, would this maybe be better: https://github.com/dbsrgits/dbix-class/blob/current/blead/lib/DBIx/Class/Optional/Dependencies.pm#L1-L10 ? The idea being only to turn on warnings and strict when in release testing so that potential problems can be found in development, however avoiding the startup costs of the warnings and strict pragmas. What do you think? Would you like me to submit a PR along these lines?

vadrer commented 8 years ago

yes, I am not a great fan of stuffing "use strict; warnings" everywhere I encounter, :) usually I do "use strict;" sometimes I avoid it on purpose, the crucial point here - this "use strict; " stuff is under control

I would suggest 2 items where to focus, I, there are lot of bogus "FAIL" reports due to lack of right tcl/tk on system, i.e. need better failback in this case II, some time ago I've recieved this mail WRT Tcl-Tk module, does it applies to Tcl?II

Thu May 21 07:42:31 2015: Request 104599 was acted upon. Transaction: Ticket created by CHORNY Queue: Tcl-Tk Subject: old META.yml type Broken in: 1.04 Severity: Normal Owner: Nobody Requestors: CHORNY@cpan.org Status: new Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=104599 >

Please upgrade ExtUtils::MakeMaker and release new version. Old versions of ExtUtils::MakeMaker cannot use new fields like 'repository' or 'LICENSE'.

And your META.yml has no 'version' field.

----thanks for your understanding and help :)

paultcochrane commented 8 years ago

With respect to item I: I've seen the failures due to missing Tcl development libraries and have been thinking about how to solve this better. Concerning item II: I can open a new ticket for this so that it can be discussed in isolation.

This leads me to a new question: where do you prefer tickets to be opened or worked on? On RT or on GitHub?

vadrer commented 8 years ago

II) - as you prefer... maybe here on GITHUB is good. I) thanks in advance :)

vadrer commented 8 years ago

no, we don't have 'use warnings' at all, we support 5.6.0 according to META.yml and according to design of the module... moreover, 'warnings.pm' contains strange code which is not at my TOP100 list, unfortunately:

package warnings;

our $VERSION = "1.34";

Verify that we're called correctly so that warnings will work.

see also strict.pm.

unless ( FILE =~ /(^|[\/])\Q${PACKAGE}\E.pmc?$/ ) { my (undef, $f, $l) = caller; die("Incorrect use of pragma '${PACKAGE}' at $f line $l.\n"); }

I am not using that.... :(

paultcochrane commented 8 years ago

Hrm, that code does look strange. And yet it's considered best practice to use both warnings and strict. Weird.

By the way: I'll use RT for more tickets, since there haven't been any issues posted on GitHub as yet, and all other trouble tickets are hosted on RT. Better to keep everything in one place :-)

vadrer commented 8 years ago

damage to strict.pm and warnings.pm was done in 5.12 $ git blame strict.pm a0d0e21e (Larry Wall 1994-10-17 23:00:00 +0000 1) package strict; a0d0e21e (Larry Wall 1994-10-17 23:00:00 +0000 2) 55fe01c5 (Doug Bell 2015-12-08 09:35:15 -0600 3) $strict::VERSION = "1.10"; e3def60f (Johan Vromans 2007-04-25 18:12:58 +0200 4) e3def60f (Johan Vromans 2007-04-25 18:12:58 +0200 5) # Verify that we're called correctly so that strictures will 5108dc18 (Rafael Garcia-Suarez 2007-10-10 08:17:07 +0000 6) unless ( FILE =~ /(^|[\/])\Q${PACKAGE}\E.pmc?$/ ) e3def60f (Johan Vromans 2007-04-25 18:12:58 +0200 7) # Can't use Carp, since Carp uses us! e3def60f (Johan Vromans 2007-04-25 18:12:58 +0200 8) my (undef, $f, $l) = caller; 5108dc18 (Rafael Garcia-Suarez 2007-10-10 08:17:07 +0000 9) die("Incorrect use of pragma '${PACKAGE}' at $f line e3def60f (Johan Vromans 2007-04-25 18:12:58 +0200 10) }

I use strict.pm but enter warnings.pm manually via -w flag, and I do not see any reason to introduce those in t/