New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add and update strictures #6

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@paultcochrane
Contributor

paultcochrane commented Jan 2, 2016

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!

paultcochrane added some commits Jan 2, 2016

Add warnings and strict pragmas
This adds the `warnings` and `strict` pragmas to files previously missing
these.  Barewords are now strings, where appropriate and all variables
requiring the `my` keyword have been updated as necessary.
Convert -w shebang line flag to warnings pragma
It is current best practice to use the `warnings` pragma rather than the
`-w` flag to the `perl` binary in the script's shebang line; hence updating
the code appropriately in this commit.
@vadrer

This comment has been minimized.

Show comment
Hide comment
@vadrer

vadrer Jan 8, 2016

Collaborator

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

Collaborator

vadrer commented Jan 8, 2016

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

This comment has been minimized.

Show comment
Hide comment
@paultcochrane

paultcochrane Jan 8, 2016

Contributor

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

Contributor

paultcochrane commented Jan 8, 2016

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

@paultcochrane

This comment has been minimized.

Show comment
Hide comment
@paultcochrane

paultcochrane Jan 8, 2016

Contributor

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?

Contributor

paultcochrane commented Jan 8, 2016

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

This comment has been minimized.

Show comment
Hide comment
@vadrer

vadrer Jan 9, 2016

Collaborator

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

Collaborator

vadrer commented Jan 9, 2016

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

This comment has been minimized.

Show comment
Hide comment
@paultcochrane

paultcochrane Jan 11, 2016

Contributor

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?

Contributor

paultcochrane commented Jan 11, 2016

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

This comment has been minimized.

Show comment
Hide comment
@vadrer

vadrer Jan 12, 2016

Collaborator

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

Collaborator

vadrer commented Jan 12, 2016

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

@vadrer

This comment has been minimized.

Show comment
Hide comment
@vadrer

vadrer Jan 13, 2016

Collaborator

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

Collaborator

vadrer commented Jan 13, 2016

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

This comment has been minimized.

Show comment
Hide comment
@paultcochrane

paultcochrane Jan 15, 2016

Contributor

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

Contributor

paultcochrane commented Jan 15, 2016

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

This comment has been minimized.

Show comment
Hide comment
@vadrer

vadrer Jan 15, 2016

Collaborator

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/

Collaborator

vadrer commented Jan 15, 2016

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/

@vadrer vadrer closed this Feb 12, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment