Skip to content
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

use if - behaviour does not match documentation #16365

Closed
p5pRT opened this issue Jan 17, 2018 · 30 comments
Closed

use if - behaviour does not match documentation #16365

p5pRT opened this issue Jan 17, 2018 · 30 comments

Comments

@p5pRT
Copy link

@p5pRT p5pRT commented Jan 17, 2018

Migrated from rt.perl.org#132732 (status was 'resolved')

Searchable as RT132732$

@p5pRT
Copy link
Author

@p5pRT p5pRT commented Jan 17, 2018

From @sisyphus

Hi,

The documentation states​:

<quote>
The use of => above provides necessary quoting of MODULE . If you don't use
the fat comma (eg you don't have any ARGUMENTS), then you'll need to quote
the MODULE.
</quote>

But the "use of => above provides necessary quoting of MODULE" only if​:
1) strictures are not enabled
&&
2) MODULE does not contain any occurrences of "​::".

For example, on MS Windows with 5.27.7 and 5.26.0​:

##########################################
C​:\>perl -Mstrict -le "use if 1, POSIX => qw(DBL_MAX);"

C​:\>perl -Mstrict -le "use if 1, Digest​::SHA => qw(sha1);"
Bareword "Digest​::SHA" not allowed while "strict subs" in use at -e line 1.
Execution of -e aborted due to compilation errors.

C​:\>perl -Mstrict -le "use if 1, Non​::Existent => qw(crap);"
Bareword "Non​::Existent" not allowed while "strict subs" in use at -e line
1.
Execution of -e aborted due to compilation errors.

C​:\>perl -Mstrict -le "use if 0, Non​::Existent => qw(crap);"
Bareword "Non​::Existent" not allowed while "strict subs" in use at -e line
1.
Execution of -e aborted due to compilation errors.

C​:\>
##########################################

But if we quote any MODULE that contains any"​::", then all works as
expected​:

##########################################
C​:\>perl -Mstrict -le "use if 1, 'Digest​::SHA' => qw(sha1);"

C​:\>perl -Mstrict -le "use if 1, 'Non​::Existent' => qw(crap);"
Can't locate Non/Existent.pm in @​INC (you may need to install the
Non​::Existent
module) (@​INC contains​: C​:/_64/blead-5.27.7/site/lib
C​:/_64/blead-5.27.7/lib) at
C​:/_64/blead-5.27.7/lib/if.pm line 15.
BEGIN failed--compilation aborted at -e line 1.

C​:\>perl -Mstrict -le "use if 0, 'Non​::Existent' => qw(crap);"

C​:\>
##########################################

The problem goes away if we don't load strict.pm.
Also, deferring the loading od strict.pm until after *all* "use if ..."
statements have been made removes the problem.

If we don't want to mess with this behaviour of if.pm, then I think these
caveats involving if.pm should be documented eg​:

##########################################

Inline Patch
--- if.pm_orig  2018-01-17 12:11:48 +1100
+++ if.pm       2018-01-17 12:33:43 +1100
@@ -50,6 +50,10 @@
If you don't use the fat comma (eg you don't have any ARGUMENTS), then you'll need to quote the MODULE\.

+NOTE​: If strictures are enabled and C<MODULE> contains any
+occurrences of C<< :​: >> then you'll need to quote the MODULE, even
+if you have used the fat comma.
+
If you wanted ARGUMENTS to be an empty list, i.e. have the effect of​:

  use MODULE ();

##########################################

Cheers,
Rob

@p5pRT
Copy link
Author

@p5pRT p5pRT commented Jan 23, 2018

From @jkeenan

On Wed, 17 Jan 2018 02​:57​:55 GMT, sisyphus wrote​:

Hi,

The documentation states​:

<quote>
The use of => above provides necessary quoting of MODULE . If you don't use
the fat comma (eg you don't have any ARGUMENTS), then you'll need to quote
the MODULE.
</quote>

But the "use of => above provides necessary quoting of MODULE" only if​:
1) strictures are not enabled
&&
2) MODULE does not contain any occurrences of "​::".

For example, on MS Windows with 5.27.7 and 5.26.0​:

##########################################
C​:\>perl -Mstrict -le "use if 1, POSIX => qw(DBL_MAX);"

C​:\>perl -Mstrict -le "use if 1, Digest​::SHA => qw(sha1);"
Bareword "Digest​::SHA" not allowed while "strict subs" in use at -e line 1.
Execution of -e aborted due to compilation errors.

C​:\>perl -Mstrict -le "use if 1, Non​::Existent => qw(crap);"
Bareword "Non​::Existent" not allowed while "strict subs" in use at -e line
1.
Execution of -e aborted due to compilation errors.

C​:\>perl -Mstrict -le "use if 0, Non​::Existent => qw(crap);"
Bareword "Non​::Existent" not allowed while "strict subs" in use at -e line
1.
Execution of -e aborted due to compilation errors.

C​:\>
##########################################

But if we quote any MODULE that contains any"​::", then all works as
expected​:

##########################################
C​:\>perl -Mstrict -le "use if 1, 'Digest​::SHA' => qw(sha1);"

C​:\>perl -Mstrict -le "use if 1, 'Non​::Existent' => qw(crap);"
Can't locate Non/Existent.pm in @​INC (you may need to install the
Non​::Existent
module) (@​INC contains​: C​:/_64/blead-5.27.7/site/lib
C​:/_64/blead-5.27.7/lib) at
C​:/_64/blead-5.27.7/lib/if.pm line 15.
BEGIN failed--compilation aborted at -e line 1.

C​:\>perl -Mstrict -le "use if 0, 'Non​::Existent' => qw(crap);"

C​:\>
##########################################

The problem goes away if we don't load strict.pm.
Also, deferring the loading od strict.pm until after *all* "use if ..."
statements have been made removes the problem.

If we don't want to mess with this behaviour of if.pm, then I think these
caveats involving if.pm should be documented eg​:

##########################################
--- if.pm_orig 2018-01-17 12​:11​:48 +1100
+++ if.pm 2018-01-17 12​:33​:43 +1100
@​@​ -50,6 +50,10 @​@​
If you don't use the fat comma (eg you don't have any ARGUMENTS),
then you'll need to quote the MODULE.

+NOTE​: If strictures are enabled and C<MODULE> contains any
+occurrences of C<< :​: >> then you'll need to quote the MODULE, even
+if you have used the fat comma.
+
If you wanted ARGUMENTS to be an empty list, i.e. have the effect of​:

 use MODULE \(\);

##########################################

Cheers,
Rob

1. I confirm your findings.

2. There are currently 10 unit tests in dist/if/t/if.t, all of which are run under 'no strict;'. If you duplicate those tests and run them under 'use strict;', 3 of the 10 immediately fail​:
#####
not ok 11 - "use if" with a false condition, fake pragma
# Failed test '"use if" with a false condition, fake pragma'
# at t/if.t line 47.
# got​: undef
# expected​: '12'
not ok 12 - "use if" with a false condition and a pragma
# Failed test '"use if" with a false condition and a pragma'
# at t/if.t line 49.
# got​: undef
# expected​: '12'
not ok 13 - "use if" with a true condition, fake pragma
# Failed test '"use if" with a true condition, fake pragma'
# at t/if.t line 52.
# got​: undef
# expected​: '12'
#####

3. The problem appears to be specific to 'use strict "subs";'. The following DWIM​:
#####
use strict;
no strict "subs";
use if 1, POSIX => qw(​:errno_h :fcntl_h);
use if 1, Digest​::SHA => qw(sha1);
use if 1, Non​::Existent, qw(foo);
#####
use strict;
no strict "subs";
use if 1, POSIX, qw(​:errno_h :fcntl_h);
use if 1, Digest​::SHA, qw(sha1);
use if 1, Non​::Existent, qw(foo);
#####

So the problem is not specific to use of the fat arrow. Indeed, the use of '=>' in the module's SYNOPSIS and DESCRIPTION is misleading. So if we revise the documentation, we should use commas rather than fat arrows.

But perhaps we need more than just doc fixes.

Thank you very much.

--
James E Keenan (jkeenan@​cpan.org)

@p5pRT
Copy link
Author

@p5pRT p5pRT commented Jan 23, 2018

The RT System itself - Status changed from 'new' to 'open'

@p5pRT
Copy link
Author

@p5pRT p5pRT commented Jan 23, 2018

From @sisyphus

-----Original Message-----
From​: James E Keenan via RT
Sent​: Wednesday, January 24, 2018 2​:45 AM
To​: OtherRecipients of perl Ticket #132732​:
Cc​: perl5-porters@​perl.org
Subject​: [perl #132732] use if - behaviour does not match documentation

On Wed, 17 Jan 2018 02​:57​:55 GMT, sisyphus wrote​:

Hi,

The documentation states​:

<quote>
The use of => above provides necessary quoting of MODULE . If you don't
use
the fat comma (eg you don't have any ARGUMENTS), then you'll need to
quote
the MODULE.
</quote>

But the "use of => above provides necessary quoting of MODULE" only if​:
1) strictures are not enabled
&&
2) MODULE does not contain any occurrences of "​::".

....

3. The problem appears to be specific to 'use strict "subs";'. The
following DWIM​:
#####
use strict;
no strict "subs";
use if 1, POSIX => qw(​:errno_h :fcntl_h);
use if 1, Digest​::SHA => qw(sha1);
use if 1, Non​::Existent, qw(foo);
#####
use strict;
no strict "subs";
use if 1, POSIX, qw(​:errno_h :fcntl_h);
use if 1, Digest​::SHA, qw(sha1);
use if 1, Non​::Existent, qw(foo);
#####

So the problem is not specific to use of the fat arrow. Indeed, the use
of '=>' in the module's SYNOPSIS and DESCRIPTION is misleading. So if we
revise the documentation, we should use commas rather than fat arrows.

Yes, that's a better appraisal.
It seems the only time the fat arrow helps is when strict "subs" is
enabled - as it then allows 'use if 1, POSIX => qw(​:errno_h :fcntl_h);' to
work.

So maybe the docs could just not mention the fat comma option, and specify
that the module name needs to be placed in quotes if strict "subs" is
enabled.
And then we just wait for someone to point out that you don't actually need
to quote the module name when strict subs are enabled, so long as you use
the fat comma && the module name doesn't contain "​::" ;-)

But perhaps we need more than just doc fixes.

Perhaps - though I'd personally be quite happy with just a doc fix.

Cheers,
Rob

@p5pRT
Copy link
Author

@p5pRT p5pRT commented Jan 24, 2018

From @Grinnz

On Tue, Jan 23, 2018 at 6​:38 PM, <sisyphus1@​optusnet.com.au> wrote​:

-----Original Message----- From​: James E Keenan via RT
Sent​: Wednesday, January 24, 2018 2​:45 AM
To​: OtherRecipients of perl Ticket #132732​:
Cc​: perl5-porters@​perl.org
Subject​: [perl #132732] use if - behaviour does not match documentation

On Wed, 17 Jan 2018 02​:57​:55 GMT, sisyphus wrote​:

Hi,

The documentation states​:

<quote>
The use of => above provides necessary quoting of MODULE . If you don't
use
the fat comma (eg you don't have any ARGUMENTS), then you'll need to
quote
the MODULE.
</quote>

But the "use of => above provides necessary quoting of MODULE" only if​:
1) strictures are not enabled
&&
2) MODULE does not contain any occurrences of "​::".

....

3. The problem appears to be specific to 'use strict "subs";'. The

following DWIM​:
#####
use strict;
no strict "subs";
use if 1, POSIX => qw(​:errno_h :fcntl_h);
use if 1, Digest​::SHA => qw(sha1);
use if 1, Non​::Existent, qw(foo);
#####
use strict;
no strict "subs";
use if 1, POSIX, qw(​:errno_h :fcntl_h);
use if 1, Digest​::SHA, qw(sha1);
use if 1, Non​::Existent, qw(foo);
#####

So the problem is not specific to use of the fat arrow. Indeed, the use
of '=>' in the module's SYNOPSIS and DESCRIPTION is misleading. So if we
revise the documentation, we should use commas rather than fat arrows.

Yes, that's a better appraisal.
It seems the only time the fat arrow helps is when strict "subs" is
enabled - as it then allows 'use if 1, POSIX => qw(​:errno_h :fcntl_h);' to
work.

So maybe the docs could just not mention the fat comma option, and specify
that the module name needs to be placed in quotes if strict "subs" is
enabled.
And then we just wait for someone to point out that you don't actually need
to quote the module name when strict subs are enabled, so long as you use
the fat comma && the module name doesn't contain "​::" ;-)

But perhaps we need more than just doc fixes.

Perhaps - though I'd personally be quite happy with just a doc fix.

Cheers,
Rob

I think it would be better to have the docs show the module name in quotes
every time. Use of the fat comma vs a regular comma is then just a
stylistic choice, and we are showing strict-safe examples as we should.

-Dan

@p5pRT
Copy link
Author

@p5pRT p5pRT commented Jan 24, 2018

From @jkeenan

On Wed, 24 Jan 2018 00​:09​:26 GMT, grinnz@​gmail.com wrote​:

On Tue, Jan 23, 2018 at 6​:38 PM, <sisyphus1@​optusnet.com.au> wrote​:

-----Original Message----- From​: James E Keenan via RT
Sent​: Wednesday, January 24, 2018 2​:45 AM
To​: OtherRecipients of perl Ticket #132732​:
Cc​: perl5-porters@​perl.org
Subject​: [perl #132732] use if - behaviour does not match
documentation

On Wed, 17 Jan 2018 02​:57​:55 GMT, sisyphus wrote​:

Hi,

The documentation states​:

<quote>
The use of => above provides necessary quoting of MODULE . If you
don't
use
the fat comma (eg you don't have any ARGUMENTS), then you'll need
to
quote
the MODULE.
</quote>

But the "use of => above provides necessary quoting of MODULE" only
if​:
1) strictures are not enabled
&&
2) MODULE does not contain any occurrences of "​::".

....

3. The problem appears to be specific to 'use strict "subs";'. The

following DWIM​:
#####
use strict;
no strict "subs";
use if 1, POSIX => qw(​:errno_h :fcntl_h);
use if 1, Digest​::SHA => qw(sha1);
use if 1, Non​::Existent, qw(foo);
#####
use strict;
no strict "subs";
use if 1, POSIX, qw(​:errno_h :fcntl_h);
use if 1, Digest​::SHA, qw(sha1);
use if 1, Non​::Existent, qw(foo);
#####

So the problem is not specific to use of the fat arrow. Indeed, the
use
of '=>' in the module's SYNOPSIS and DESCRIPTION is misleading. So
if we
revise the documentation, we should use commas rather than fat
arrows.

Yes, that's a better appraisal.
It seems the only time the fat arrow helps is when strict "subs" is
enabled - as it then allows 'use if 1, POSIX => qw(​:errno_h
:fcntl_h);' to
work.

So maybe the docs could just not mention the fat comma option, and
specify
that the module name needs to be placed in quotes if strict "subs" is
enabled.
And then we just wait for someone to point out that you don't
actually need
to quote the module name when strict subs are enabled, so long as you
use
the fat comma && the module name doesn't contain "​::" ;-)

But perhaps we need more than just doc fixes.

Perhaps - though I'd personally be quite happy with just a doc fix.

Cheers,
Rob

I think it would be better to have the docs show the module name in
quotes
every time. Use of the fat comma vs a regular comma is then just a
stylistic choice, and we are showing strict-safe examples as we
should.

-Dan

Sounds good. I'll prepare a patch tomorrow.

--
James E Keenan (jkeenan@​cpan.org)

@p5pRT
Copy link
Author

@p5pRT p5pRT commented Jan 24, 2018

From @jkeenan

On Wed, 24 Jan 2018 03​:39​:01 GMT, jkeenan wrote​:

On Wed, 24 Jan 2018 00​:09​:26 GMT, grinnz@​gmail.com wrote​:

On Tue, Jan 23, 2018 at 6​:38 PM, <sisyphus1@​optusnet.com.au> wrote​:

-----Original Message----- From​: James E Keenan via RT
Sent​: Wednesday, January 24, 2018 2​:45 AM
To​: OtherRecipients of perl Ticket #132732​:
Cc​: perl5-porters@​perl.org
Subject​: [perl #132732] use if - behaviour does not match
documentation

On Wed, 17 Jan 2018 02​:57​:55 GMT, sisyphus wrote​:

Hi,

The documentation states​:

<quote>
The use of => above provides necessary quoting of MODULE . If you
don't
use
the fat comma (eg you don't have any ARGUMENTS), then you'll need
to
quote
the MODULE.
</quote>

But the "use of => above provides necessary quoting of MODULE" only
if​:
1) strictures are not enabled
&&
2) MODULE does not contain any occurrences of "​::".

....

3. The problem appears to be specific to 'use strict "subs";'. The

following DWIM​:
#####
use strict;
no strict "subs";
use if 1, POSIX => qw(​:errno_h :fcntl_h);
use if 1, Digest​::SHA => qw(sha1);
use if 1, Non​::Existent, qw(foo);
#####
use strict;
no strict "subs";
use if 1, POSIX, qw(​:errno_h :fcntl_h);
use if 1, Digest​::SHA, qw(sha1);
use if 1, Non​::Existent, qw(foo);
#####

So the problem is not specific to use of the fat arrow. Indeed, the
use
of '=>' in the module's SYNOPSIS and DESCRIPTION is misleading. So
if we
revise the documentation, we should use commas rather than fat
arrows.

Yes, that's a better appraisal.
It seems the only time the fat arrow helps is when strict "subs" is
enabled - as it then allows 'use if 1, POSIX => qw(​:errno_h
:fcntl_h);' to
work.

So maybe the docs could just not mention the fat comma option, and
specify
that the module name needs to be placed in quotes if strict "subs" is
enabled.
And then we just wait for someone to point out that you don't
actually need
to quote the module name when strict subs are enabled, so long as you
use
the fat comma && the module name doesn't contain "​::" ;-)

But perhaps we need more than just doc fixes.

Perhaps - though I'd personally be quite happy with just a doc fix.

Cheers,
Rob

I think it would be better to have the docs show the module name in
quotes
every time. Use of the fat comma vs a regular comma is then just a
stylistic choice, and we are showing strict-safe examples as we
should.

-Dan

Sounds good. I'll prepare a patch tomorrow.

This has proven to be trickier than I anticipated -- though not the documentation.

I figured it would be a good idea to add tests for every claim made in the documentation. So I started to add tests to dist/if/t/if.t. (Got some advice on #p5p about this from ilmari, haarg and Abigail.) I wrote what I thought were some plausible tests for the 'no if CONDITION, MODULE => ARGUMENTS;' case only to discover that the 'no' did not appear to have any impact. See patch attached. When I run this against blead, I get​:

#####
$ cd t;./perl harness -v ../dist/if/t/if.t; cd -

ok 1 - "use if" with a false condition, fake pragma
ok 2 - "use if" with a false condition and a pragma
ok 3 - "use if" with a true condition, fake pragma
ok 4 - "use if" with a true condition and a pragma
ok 5 - expected error message
ok 6 - "use if" with open
ok 7 - Too few args to 'use if' returns <undef>
ok 8 - ... and returns correct error
ok 9 - Too few args to 'no if' returns <undef>
ok 10 - ... and returns correct error
AAA​: 1311768467284833424
ok 11 - Cannot hex
ok 12 - Cannot oct
not ok 13 - Cannot hex
# Failed test 'Cannot hex'
# at t/if.t line 57.
not ok 14 - Cannot oct
# Failed test 'Cannot oct'
# at t/if.t line 58.
BBB​: 1311768467284833424
CCC​: 1311768467284833424
ok 15 - Can hex
# Looks like you failed 2 tests of 16.
ok 16 - Can oct
Dubious, test returned 2 (wstat 512, 0x200)
Failed 2/16 subtests

Test Summary Report


../dist/if/t/if.t (Wstat​: 512 Tests​: 16 Failed​: 2)
  Failed tests​: 13-14
  Non-zero exit status​: 2
Files=1, Tests=16, 0 wallclock secs ( 0.01 usr 0.00 sys + 0.08 cusr 0.00 csys = 0.09 CPU)
Result​: FAIL
#####

Can anyone advise on 'no if'?

Thank you very much.

--
James E Keenan (jkeenan@​cpan.org)

@p5pRT
Copy link
Author

@p5pRT p5pRT commented Jan 24, 2018

From @jkeenan

0001-Clarify-documentation-for-if-module.patch
From 1bd4c7ee474965c923e3d6bc19c0b08bc6053701 Mon Sep 17 00:00:00 2001
From: James E Keenan <jkeenan@cpan.org>
Date: Tue, 23 Jan 2018 10:46:32 -0500
Subject: [PATCH] Clarify documentation for 'if' module.

Add tests for "strict 'subs'" -- but tests for 'no CONDITION' are not working.
---
 dist/if/if.pm  | 28 ++++++++++------------
 dist/if/t/if.t | 73 ++++++++++++++++++++++++++++++++++++++++------------------
 2 files changed, 63 insertions(+), 38 deletions(-)

diff --git a/dist/if/if.pm b/dist/if/if.pm
index d1cbd00..edc5a23 100644
--- a/dist/if/if.pm
+++ b/dist/if/if.pm
@@ -1,6 +1,6 @@
 package if;
 
-$VERSION = '0.0607';
+$VERSION = '0.0608';
 
 sub work {
   my $method = shift() ? 'import' : 'unimport';
@@ -29,26 +29,22 @@ if - C<use> a Perl module if a condition holds (also can C<no> a module)
 
 =head1 SYNOPSIS
 
-  use if CONDITION, MODULE => ARGUMENTS;
-  no if CONDITION, MODULE => ARGUMENTS;
+    use if CONDITION, "MODULE", ARGUMENTS;
+    no  if CONDITION, "MODULE", ARGUMENTS;
 
 =head1 DESCRIPTION
 
 The C<if> module is used to conditionally load or unload another module.
 The construct
 
-  use if CONDITION, MODULE => ARGUMENTS;
+    use if CONDITION, "MODULE", ARGUMENTS;
 
-will load MODULE only if CONDITION evaluates to true.
-The above statement has no effect unless C<CONDITION> is true.
-If the CONDITION does evaluate to true, then the above line has
-the same effect as:
+will load MODULE only if CONDITION evaluates to true.  The above statement has
+no effect unless C<CONDITION> is true.  (The module name must be quoted when
+C<'use strict "subs";'> is in effect.) If the CONDITION does evaluate to true,
+then the above line has the same effect as:
 
-  use MODULE ARGUMENTS;
-
-The use of C<< => >> above provides necessary quoting of C<MODULE>.
-If you don't use the fat comma (eg you don't have any ARGUMENTS),
-then you'll need to quote the MODULE.
+    use MODULE ARGUMENTS;
 
 If you wanted ARGUMENTS to be an empty list, i.e. have the effect of:
 
@@ -63,7 +59,7 @@ exactly this effect, at compile time, with:
 
 The following line is taken from the testsuite for L<File::Map>:
 
-  use if $^O ne 'MSWin32', POSIX => qw/setlocale LC_ALL/;
+    use if $^O ne 'MSWin32', POSIX => qw/setlocale LC_ALL/;
 
 If run on any operating system other than Windows,
 this will import the functions C<setlocale> and C<LC_ALL> from L<POSIX>.
@@ -71,7 +67,7 @@ On Windows it does nothing.
 
 The following is used to L<deprecate> core modules beyond a certain version of Perl:
 
-  use if $] > 5.016, 'deprecate';
+    use if $] > 5.016, 'deprecate';
 
 This line is taken from L<Text::Soundex> 3.04,
 and marks it as deprecated beyond Perl 5.16.
@@ -85,7 +81,7 @@ unless you've installed a more recent version of L<Text::Soundex> from CPAN.
 
 You can also specify to NOT use something:
 
- no if $] ge 5.021_006, warnings => "locale";
+    no if $] ge 5.021_006, warnings => "locale";
 
 This warning category was added in the specified Perl version (a development
 release).  Without the C<'if'>, trying to use it in an earlier release would
diff --git a/dist/if/t/if.t b/dist/if/t/if.t
index 4a2b351..4854b7d 100644
--- a/dist/if/t/if.t
+++ b/dist/if/t/if.t
@@ -1,9 +1,9 @@
 #!./perl
 
 use strict;
-use Test::More tests => 10;
+use Test::More tests => 16;
 
-my $v_plus = $] + 1;
+my $v_plus  = $] + 1;
 my $v_minus = $] - 1;
 
 unless (eval 'use open ":std"; 1') {
@@ -12,29 +12,58 @@ unless (eval 'use open ":std"; 1') {
   eval 'sub open::foo{}';		# Just in case...
 }
 
-no strict;
+{
+    no strict;
 
-is( eval "use if ($v_minus > \$]), strict => 'subs'; \${'f'} = 12", 12,
-    '"use if" with a false condition, fake pragma');
-is( eval "use if ($v_minus > \$]), strict => 'refs'; \${'f'} = 12", 12,
-    '"use if" with a false condition and a pragma');
+    is( eval "use if ($v_minus > \$]), strict => 'subs'; \${'f'} = 12", 12,
+        '"use if" with a false condition, fake pragma');
+    is( eval "use if ($v_minus > \$]), strict => 'refs'; \${'f'} = 12", 12,
+        '"use if" with a false condition and a pragma');
 
-is( eval "use if ($v_plus > \$]), strict => 'subs'; \${'f'} = 12", 12,
-    '"use if" with a true condition, fake pragma');
+    is( eval "use if ($v_plus > \$]), strict => 'subs'; \${'f'} = 12", 12,
+        '"use if" with a true condition, fake pragma');
 
-is( eval "use if ($v_plus > \$]), strict => 'refs'; \${'f'} = 12", undef,
-    '"use if" with a true condition and a pragma');
-like( $@, qr/while "strict refs" in use/, 'expected error message'),
+    is( eval "use if ($v_plus > \$]), strict => 'refs'; \${'f'} = 12", undef,
+        '"use if" with a true condition and a pragma');
+    like( $@, qr/while "strict refs" in use/, 'expected error message'),
 
-# Old version had problems with the module name 'open', which is a keyword too
-# Use 'open' =>, since pre-5.6.0 could interpret differently
-is( (eval "use if ($v_plus > \$]), 'open' => IN => ':crlf'; 12" || 0), 12,
-    '"use if" with open');
+    # Old version had problems with the module name 'open', which is a keyword too
+    # Use 'open' =>, since pre-5.6.0 could interpret differently
+    is( (eval "use if ($v_plus > \$]), 'open' => IN => ':crlf'; 12" || 0), 12,
+        '"use if" with open');
 
-is(eval "use if ($v_plus > \$])", undef,
-   "Too few args to 'use if' returns <undef>");
-like($@, qr/Too few arguments to 'use if'/, "  ... and returns correct error");
+    is(eval "use if ($v_plus > \$])", undef,
+       "Too few args to 'use if' returns <undef>");
+    like($@, qr/Too few arguments to 'use if'/, "  ... and returns correct error");
 
-is(eval "no if ($v_plus > \$])", undef,
-   "Too few args to 'no if' returns <undef>");
-like($@, qr/Too few arguments to 'no if'/, "  ... and returns correct error");
+    is(eval "no if ($v_plus > \$])", undef,
+       "Too few args to 'no if' returns <undef>");
+    like($@, qr/Too few arguments to 'no if'/, "  ... and returns correct error");
+}
+
+{
+    note(q|RT 132732: strict 'subs'|);
+    use strict "subs";
+
+    {
+        eval "use if (0 > 1), q|bigrat|, qw(hex oct);";
+        ok (! bigrat->can('hex'), "Cannot hex");
+        ok (! bigrat->can('oct'), "Cannot oct");
+        print STDERR "AAA: ", hex("0x1234567890123490"),"\n";
+    }
+
+    {
+        eval "no if (1 > 0), q|bigrat|, qw(hex oct);";
+        ok (! bigrat->can('hex'), "Cannot hex");
+        ok (! bigrat->can('oct'), "Cannot oct");
+        print STDERR "BBB: ", hex("0x1234567890123490"),"\n";
+    }
+
+    {
+        eval "use if (1 > 0), q|bigrat|, qw(hex oct);";
+        ok (bigrat->can('hex'), "Can hex");
+        ok (bigrat->can('oct'), "Can oct");
+        print STDERR "CCC: ", hex("0x1234567890123490"),"\n";
+    }
+
+}
-- 
2.7.4

@p5pRT
Copy link
Author

@p5pRT p5pRT commented Jan 24, 2018

From @Grinnz

From the source code, 'no if' appears to do the same thing as 'use if' but
runs unimport instead of import. It's not a negation of the condition. This
is consistent with 'no' and 'use' but the negation is not explained very
well IMO.

-Dan

On Wed, Jan 24, 2018 at 11​:02 AM, James E Keenan via RT <
perlbug-followup@​perl.org> wrote​:

On Wed, 24 Jan 2018 03​:39​:01 GMT, jkeenan wrote​:

On Wed, 24 Jan 2018 00​:09​:26 GMT, grinnz@​gmail.com wrote​:

On Tue, Jan 23, 2018 at 6​:38 PM, <sisyphus1@​optusnet.com.au> wrote​:

-----Original Message----- From​: James E Keenan via RT
Sent​: Wednesday, January 24, 2018 2​:45 AM
To​: OtherRecipients of perl Ticket #132732​:
Cc​: perl5-porters@​perl.org
Subject​: [perl #132732] use if - behaviour does not match
documentation

On Wed, 17 Jan 2018 02​:57​:55 GMT, sisyphus wrote​:

Hi,

The documentation states​:

<quote>
The use of => above provides necessary quoting of MODULE . If you
don't
use
the fat comma (eg you don't have any ARGUMENTS), then you'll need
to
quote
the MODULE.
</quote>

But the "use of => above provides necessary quoting of MODULE" only
if​:
1) strictures are not enabled
&&
2) MODULE does not contain any occurrences of "​::".

....

3. The problem appears to be specific to 'use strict "subs";'. The

following DWIM​:
#####
use strict;
no strict "subs";
use if 1, POSIX => qw(​:errno_h :fcntl_h);
use if 1, Digest​::SHA => qw(sha1);
use if 1, Non​::Existent, qw(foo);
#####
use strict;
no strict "subs";
use if 1, POSIX, qw(​:errno_h :fcntl_h);
use if 1, Digest​::SHA, qw(sha1);
use if 1, Non​::Existent, qw(foo);
#####

So the problem is not specific to use of the fat arrow. Indeed, the
use
of '=>' in the module's SYNOPSIS and DESCRIPTION is misleading. So
if we
revise the documentation, we should use commas rather than fat
arrows.

Yes, that's a better appraisal.
It seems the only time the fat arrow helps is when strict "subs" is
enabled - as it then allows 'use if 1, POSIX => qw(​:errno_h
:fcntl_h);' to
work.

So maybe the docs could just not mention the fat comma option, and
specify
that the module name needs to be placed in quotes if strict "subs" is
enabled.
And then we just wait for someone to point out that you don't
actually need
to quote the module name when strict subs are enabled, so long as you
use
the fat comma && the module name doesn't contain "​::" ;-)

But perhaps we need more than just doc fixes.

Perhaps - though I'd personally be quite happy with just a doc fix.

Cheers,
Rob

I think it would be better to have the docs show the module name in
quotes
every time. Use of the fat comma vs a regular comma is then just a
stylistic choice, and we are showing strict-safe examples as we
should.

-Dan

Sounds good. I'll prepare a patch tomorrow.

This has proven to be trickier than I anticipated -- though not the
documentation.

I figured it would be a good idea to add tests for every claim made in the
documentation. So I started to add tests to dist/if/t/if.t. (Got some
advice on #p5p about this from ilmari, haarg and Abigail.) I wrote what I
thought were some plausible tests for the 'no if CONDITION, MODULE =>
ARGUMENTS;' case only to discover that the 'no' did not appear to have any
impact. See patch attached. When I run this against blead, I get​:

#####
$ cd t;./perl harness -v ../dist/if/t/if.t; cd -

ok 1 - "use if" with a false condition, fake pragma
ok 2 - "use if" with a false condition and a pragma
ok 3 - "use if" with a true condition, fake pragma
ok 4 - "use if" with a true condition and a pragma
ok 5 - expected error message
ok 6 - "use if" with open
ok 7 - Too few args to 'use if' returns <undef>
ok 8 - ... and returns correct error
ok 9 - Too few args to 'no if' returns <undef>
ok 10 - ... and returns correct error
AAA​: 1311768467284833424
ok 11 - Cannot hex
ok 12 - Cannot oct
not ok 13 - Cannot hex
# Failed test 'Cannot hex'
# at t/if.t line 57.
not ok 14 - Cannot oct
# Failed test 'Cannot oct'
# at t/if.t line 58.
BBB​: 1311768467284833424
CCC​: 1311768467284833424
ok 15 - Can hex
# Looks like you failed 2 tests of 16.
ok 16 - Can oct
Dubious, test returned 2 (wstat 512, 0x200)
Failed 2/16 subtests

Test Summary Report
-------------------
../dist/if/t/if.t (Wstat​: 512 Tests​: 16 Failed​: 2)
Failed tests​: 13-14
Non-zero exit status​: 2
Files=1, Tests=16, 0 wallclock secs ( 0.01 usr 0.00 sys + 0.08 cusr
0.00 csys = 0.09 CPU)
Result​: FAIL
#####

Can anyone advise on 'no if'?

Thank you very much.

--
James E Keenan (jkeenan@​cpan.org)

---
via perlbug​: queue​: perl5 status​: open
https://rt-archive.perl.org/perl5/Ticket/Display.html?id=132732

From 1bd4c7ee474965c923e3d6bc19c0b08bc6053701 Mon Sep 17 00​:00​:00 2001
From​: James E Keenan <jkeenan@​cpan.org>
Date​: Tue, 23 Jan 2018 10​:46​:32 -0500
Subject​: [PATCH] Clarify documentation for 'if' module.

Add tests for "strict 'subs'" -- but tests for 'no CONDITION' are not
working.
---
dist/if/if.pm | 28 ++++++++++------------
dist/if/t/if.t | 73 ++++++++++++++++++++++++++++++
++++++++++------------------
2 files changed, 63 insertions(+), 38 deletions(-)

diff --git a/dist/if/if.pm b/dist/if/if.pm
index d1cbd00..edc5a23 100644
--- a/dist/if/if.pm
+++ b/dist/if/if.pm
@​@​ -1,6 +1,6 @​@​
package if;

-$VERSION = '0.0607';
+$VERSION = '0.0608';

sub work {
my $method = shift() ? 'import' : 'unimport';
@​@​ -29,26 +29,22 @​@​ if - C<use> a Perl module if a condition holds (also
can C<no> a module)

=head1 SYNOPSIS

- use if CONDITION, MODULE => ARGUMENTS;
- no if CONDITION, MODULE => ARGUMENTS;
+ use if CONDITION, "MODULE", ARGUMENTS;
+ no if CONDITION, "MODULE", ARGUMENTS;

=head1 DESCRIPTION

The C<if> module is used to conditionally load or unload another module.
The construct

- use if CONDITION, MODULE => ARGUMENTS;
+ use if CONDITION, "MODULE", ARGUMENTS;

-will load MODULE only if CONDITION evaluates to true.
-The above statement has no effect unless C<CONDITION> is true.
-If the CONDITION does evaluate to true, then the above line has
-the same effect as​:
+will load MODULE only if CONDITION evaluates to true. The above
statement has
+no effect unless C<CONDITION> is true. (The module name must be quoted
when
+C<'use strict "subs";'> is in effect.) If the CONDITION does evaluate to
true,
+then the above line has the same effect as​:

- use MODULE ARGUMENTS;
-
-The use of C<< => >> above provides necessary quoting of C<MODULE>.
-If you don't use the fat comma (eg you don't have any ARGUMENTS),
-then you'll need to quote the MODULE.
+ use MODULE ARGUMENTS;

If you wanted ARGUMENTS to be an empty list, i.e. have the effect of​:

@​@​ -63,7 +59,7 @​@​ exactly this effect, at compile time, with​:

The following line is taken from the testsuite for L<File​::Map>​:

- use if $^O ne 'MSWin32', POSIX => qw/setlocale LC_ALL/;
+ use if $^O ne 'MSWin32', POSIX => qw/setlocale LC_ALL/;

If run on any operating system other than Windows,
this will import the functions C<setlocale> and C<LC_ALL> from L<POSIX>.
@​@​ -71,7 +67,7 @​@​ On Windows it does nothing.

The following is used to L<deprecate> core modules beyond a certain
version of Perl​:

- use if $] > 5.016, 'deprecate';
+ use if $] > 5.016, 'deprecate';

This line is taken from L<Text​::Soundex> 3.04,
and marks it as deprecated beyond Perl 5.16.
@​@​ -85,7 +81,7 @​@​ unless you've installed a more recent version of
L<Text​::Soundex> from CPAN.

You can also specify to NOT use something​:

- no if $] ge 5.021_006, warnings => "locale";
+ no if $] ge 5.021_006, warnings => "locale";

This warning category was added in the specified Perl version (a
development
release). Without the C<'if'>, trying to use it in an earlier release
would
diff --git a/dist/if/t/if.t b/dist/if/t/if.t
index 4a2b351..4854b7d 100644
--- a/dist/if/t/if.t
+++ b/dist/if/t/if.t
@​@​ -1,9 +1,9 @​@​
#!./perl

use strict;
-use Test​::More tests => 10;
+use Test​::More tests => 16;

-my $v_plus = $] + 1;
+my $v_plus = $] + 1;
my $v_minus = $] - 1;

unless (eval 'use open "​:std"; 1') {
@​@​ -12,29 +12,58 @​@​ unless (eval 'use open "​:std"; 1') {
eval 'sub open​::foo{}'; # Just in case...
}

-no strict;
+{
+ no strict;

-is( eval "use if ($v_minus > \$]), strict => 'subs'; \${'f'} = 12", 12,
- '"use if" with a false condition, fake pragma');
-is( eval "use if ($v_minus > \$]), strict => 'refs'; \${'f'} = 12", 12,
- '"use if" with a false condition and a pragma');
+ is( eval "use if ($v_minus > \$]), strict => 'subs'; \${'f'} = 12",
12,
+ '"use if" with a false condition, fake pragma');
+ is( eval "use if ($v_minus > \$]), strict => 'refs'; \${'f'} = 12",
12,
+ '"use if" with a false condition and a pragma');

-is( eval "use if ($v_plus > \$]), strict => 'subs'; \${'f'} = 12", 12,
- '"use if" with a true condition, fake pragma');
+ is( eval "use if ($v_plus > \$]), strict => 'subs'; \${'f'} = 12", 12,
+ '"use if" with a true condition, fake pragma');

-is( eval "use if ($v_plus > \$]), strict => 'refs'; \${'f'} = 12", undef,
- '"use if" with a true condition and a pragma');
-like( $@​, qr/while "strict refs" in use/, 'expected error message'),
+ is( eval "use if ($v_plus > \$]), strict => 'refs'; \${'f'} = 12",
undef,
+ '"use if" with a true condition and a pragma');
+ like( $@​, qr/while "strict refs" in use/, 'expected error message'),

-# Old version had problems with the module name 'open', which is a
keyword too
-# Use 'open' =>, since pre-5.6.0 could interpret differently
-is( (eval "use if ($v_plus > \$]), 'open' => IN => '​:crlf'; 12" || 0), 12,
- '"use if" with open');
+ # Old version had problems with the module name 'open', which is a
keyword too
+ # Use 'open' =>, since pre-5.6.0 could interpret differently
+ is( (eval "use if ($v_plus > \$]), 'open' => IN => '​:crlf'; 12" ||
0), 12,
+ '"use if" with open');

-is(eval "use if ($v_plus > \$])", undef,
- "Too few args to 'use if' returns <undef>");
-like($@​, qr/Too few arguments to 'use if'/, " ... and returns correct
error");
+ is(eval "use if ($v_plus > \$])", undef,
+ "Too few args to 'use if' returns <undef>");
+ like($@​, qr/Too few arguments to 'use if'/, " ... and returns
correct error");

-is(eval "no if ($v_plus > \$])", undef,
- "Too few args to 'no if' returns <undef>");
-like($@​, qr/Too few arguments to 'no if'/, " ... and returns correct
error");
+ is(eval "no if ($v_plus > \$])", undef,
+ "Too few args to 'no if' returns <undef>");
+ like($@​, qr/Too few arguments to 'no if'/, " ... and returns correct
error");
+}
+
+{
+ note(q|RT 132732​: strict 'subs'|);
+ use strict "subs";
+
+ {
+ eval "use if (0 > 1), q|bigrat|, qw(hex oct);";
+ ok (! bigrat->can('hex'), "Cannot hex");
+ ok (! bigrat->can('oct'), "Cannot oct");
+ print STDERR "AAA​: ", hex("0x1234567890123490"),"\n";
+ }
+
+ {
+ eval "no if (1 > 0), q|bigrat|, qw(hex oct);";
+ ok (! bigrat->can('hex'), "Cannot hex");
+ ok (! bigrat->can('oct'), "Cannot oct");
+ print STDERR "BBB​: ", hex("0x1234567890123490"),"\n";
+ }
+
+ {
+ eval "use if (1 > 0), q|bigrat|, qw(hex oct);";
+ ok (bigrat->can('hex'), "Can hex");
+ ok (bigrat->can('oct'), "Can oct");
+ print STDERR "CCC​: ", hex("0x1234567890123490"),"\n";
+ }
+
+}
--
2.7.4

@p5pRT
Copy link
Author

@p5pRT p5pRT commented Jan 24, 2018

From @jkeenan

On 01/24/2018 12​:20 PM, Dan Book wrote​:

From the source code, 'no if' appears to do the same thing as 'use if'
but runs unimport instead of import. It's not a negation of the
condition. This is consistent with 'no' and 'use' but the negation is
not explained very well IMO.

So, would a proper test then be to import specific functions, then use
'no if CONDITION, "MODULE" ARGUMENTS to unimport them?

jimk

@p5pRT
Copy link
Author

@p5pRT p5pRT commented Jan 24, 2018

From @Grinnz

On Wed, Jan 24, 2018 at 12​:33 PM, James E Keenan <jkeenan@​pobox.com> wrote​:

On 01/24/2018 12​:20 PM, Dan Book wrote​:

From the source code, 'no if' appears to do the same thing as 'use if'
but runs unimport instead of import. It's not a negation of the condition.
This is consistent with 'no' and 'use' but the negation is not explained
very well IMO.

So, would a proper test then be to import specific functions, then use 'no
if CONDITION, "MODULE" ARGUMENTS to unimport them?

That should be reasonable, but note that not many modules implement
unimport; 'no warnings' is probably the most common usage of 'no'.

-Dan

@p5pRT
Copy link
Author

@p5pRT p5pRT commented Jan 24, 2018

From @jkeenan

On Wed, 24 Jan 2018 17​:38​:14 GMT, grinnz@​gmail.com wrote​:

On Wed, Jan 24, 2018 at 12​:33 PM, James E Keenan <jkeenan@​pobox.com>
wrote​:

On 01/24/2018 12​:20 PM, Dan Book wrote​:

From the source code, 'no if' appears to do the same thing as 'use
if'
but runs unimport instead of import. It's not a negation of the
condition.
This is consistent with 'no' and 'use' but the negation is not
explained
very well IMO.

So, would a proper test then be to import specific functions, then
use 'no
if CONDITION, "MODULE" ARGUMENTS to unimport them?

That should be reasonable, but note that not many modules implement
unimport; 'no warnings' is probably the most common usage of 'no'.

-Dan

Well, the reason I used bigrat in the tests is that it does contain a 'sub unimport'.

As does 're'. But there I get even stranger results. Try this out (in the patch)​:

#####
  {
  eval "use if (0 > 1), q|re|, qw(is_regexp regexp_pattern);";
  ok (! re->can('is_regexp'), "Cannot is_regexp");
  ok (! re->can('regexp_pattern'), "Cannot regexp_pattern");
  ok ( re->can('is_regexp'), "Can is_regexp");
  ok ( re->can('regexp_pattern'), "Can regexp_pattern");
  }
#####
I get​:
#####
not ok 11 - Cannot is_regexp
# Failed test 'Cannot is_regexp'
# at t/if.t line 73.
not ok 12 - Cannot regexp_pattern
# Failed test 'Cannot regexp_pattern'
# at t/if.t line 74.
ok 13 - Can is_regexp
ok 14 - Can regexp_pattern
#####

Which I read as, "Whether or not the CONDITION evaluates to true or not, the two functions are imported from package 're'."

--
James E Keenan (jkeenan@​cpan.org)

@p5pRT
Copy link
Author

@p5pRT p5pRT commented Jan 24, 2018

From @Grinnz

On Wed, Jan 24, 2018 at 1​:09 PM, James E Keenan via RT <
perlbug-followup@​perl.org> wrote​:

As does 're'. But there I get even stranger results. Try this out (in
the patch)​:

#####
{
eval "use if (0 > 1), q|re|, qw(is_regexp regexp_pattern);";
ok (! re->can('is_regexp'), "Cannot is_regexp");
ok (! re->can('regexp_pattern'), "Cannot regexp_pattern");
ok ( re->can('is_regexp'), "Can is_regexp");
ok ( re->can('regexp_pattern'), "Can regexp_pattern");
}
#####
I get​:
#####
not ok 11 - Cannot is_regexp
# Failed test 'Cannot is_regexp'
# at t/if.t line 73.
not ok 12 - Cannot regexp_pattern
# Failed test 'Cannot regexp_pattern'
# at t/if.t line 74.
ok 13 - Can is_regexp
ok 14 - Can regexp_pattern
#####

Which I read as, "Whether or not the CONDITION evaluates to true or not,
the two functions are imported from package 're'."

This does not look like it's testing imports; it's testing whether the
is_regexp and regexp_pattern functions exist in re​::, which is always true.

-Dan

@p5pRT
Copy link
Author

@p5pRT p5pRT commented Jan 24, 2018

From @jkeenan

On Wed, 24 Jan 2018 18​:16​:40 GMT, grinnz@​gmail.com wrote​:

On Wed, Jan 24, 2018 at 1​:09 PM, James E Keenan via RT <
perlbug-followup@​perl.org> wrote​:

As does 're'. But there I get even stranger results. Try this out (in
the patch)​:

#####
{
eval "use if (0 > 1), q|re|, qw(is_regexp regexp_pattern);";
ok (! re->can('is_regexp'), "Cannot is_regexp");
ok (! re->can('regexp_pattern'), "Cannot regexp_pattern");
ok ( re->can('is_regexp'), "Can is_regexp");
ok ( re->can('regexp_pattern'), "Can regexp_pattern");
}
#####
I get​:
#####
not ok 11 - Cannot is_regexp
# Failed test 'Cannot is_regexp'
# at t/if.t line 73.
not ok 12 - Cannot regexp_pattern
# Failed test 'Cannot regexp_pattern'
# at t/if.t line 74.
ok 13 - Can is_regexp
ok 14 - Can regexp_pattern
#####

Which I read as, "Whether or not the CONDITION evaluates to true or not,
the two functions are imported from package 're'."

This does not look like it's testing imports; it's testing whether the
is_regexp and regexp_pattern functions exist in re​::, which is always true.

-Dan

Noted. But please run the attached file. To the best of my understanding of the 'if' documentation, the tests in the third block of the file should pass, i.e., they should catch exceptions and set $@​. They are not doing so? What are we missing?

Thank you very much.

--
James E Keenan (jkeenan@​cpan.org)

@p5pRT
Copy link
Author

@p5pRT p5pRT commented Jan 24, 2018

From @jkeenan

#!/usr/bin/env perl
use strict;
use warnings;
use Test​::More tests => 7;

eval "use if (1 < 0), q|re|, qw(is_regexp regexp_pattern);";
{
  my ($pat, $mods, $obj);
  $obj = qr/foo/i;

  local $@​;
  eval { ($pat,$mods) = regexp_pattern($obj); };
  like($@​, qr/^Undefined subroutine \&main​::regexp_pattern called/,
  "'if' condition evaluated false; function regexp_pattern not imported");
  local $@​;
  eval { is_regexp($obj); };
  like($@​, qr/^Undefined subroutine \&main​::is_regexp called/,
  "'if' condition evaluated false; function is_regexp not imported");
}

eval "use if (0 < 1), q|re|, qw(is_regexp regexp_pattern);";
{
  my ($pat, $mods, $obj);
  $obj = qr/foo/i;

  ($pat,$mods) = regexp_pattern($obj);
  ok(defined $pat, "'if' condition evaluated true; regexp_pattern() imported");
  ok(defined $mods, "'if' condition evaluated true; regexp_pattern() imported");
  ok(is_regexp($obj), "'if' condition evaluated true; is_regexp imported");
}

eval "no if (0 < 1), q|re|, qw(is_regexp regexp_pattern);";
{
  my ($pat, $mods, $obj);
  $obj = qr/foo/i;

  local $@​;
  eval { ($pat,$mods) = regexp_pattern($obj); };
  like($@​, qr/^Undefined subroutine \&main​::regexp_pattern called/,
  "'no' condition evaluated true; function regexp_pattern unimported");
  local $@​;
  eval { is_regexp($obj); };
  like($@​, qr/^Undefined subroutine \&main​::is_regexp called/,
  "'no' condition evaluated true; function is_regexp unimported");
}

@p5pRT
Copy link
Author

@p5pRT p5pRT commented Jan 24, 2018

From @Grinnz

On Wed, Jan 24, 2018 at 3​:12 PM, James E Keenan via RT <
perlbug-followup@​perl.org> wrote​:

Noted. But please run the attached file. To the best of my understanding
of the 'if' documentation, the tests in the third block of the file should
pass, i.e., they should catch exceptions and set $@​. They are not doing
so? What are we missing?

The 'unimport' method (actually implemented in the 'bits' method) of re.pm
does not appear to have any logic for 'unimporting' functions. It only has
reverse logic for the 'debug', 'strict', and flag parameters.

-Dan

@p5pRT
Copy link
Author

@p5pRT p5pRT commented Jan 24, 2018

From @jkeenan

On Wed, 24 Jan 2018 17​:38​:14 GMT, grinnz@​gmail.com wrote​:

That should be reasonable, but note that not many modules implement
unimport; 'no warnings' is probably the most common usage of 'no'.

-Dan

There are many instances of the 'no' function in the source code, but very few instances of the string 'no if'. See attachment for output of 'ack '\bno\s+if\b' .'. Almost every instance is testing one of Perl's special variables against some value and, as you say, most are warnings-related.

Unfortunately that isn't getting us any close to solving the problem. Is anyone familiar with where/how 'no' is implemented in the source code?

Thank you very much.

--
James E Keenan (jkeenan@​cpan.org)

@p5pRT
Copy link
Author

@p5pRT p5pRT commented Jan 24, 2018

From @jkeenan

dist/IO/t/IO.t​:52​: no if $^V >= 5.17.4, warnings => "deprecated";
dist/Devel-PPPort/parts/inc/misc​:696​: no if $^V > v5.17.9, warnings => "experimental​::lexical_topic";
dist/Devel-PPPort/t/misc.t​:63​: no if $^V > v5.17.9, warnings => "experimental​::lexical_topic";
dist/Data-Dumper/t/indent.t​:95​: no if $] < 5.011, warnings => 'deprecated';
dist/if/Changes​:7​: - Better failure message for 'no if'​: It previously always
dist/if/Changes​:8​: said 'use if', even if 'no if' was what was specified (Karl
dist/if/if.pm​:33​: no if CONDITION, MODULE => ARGUMENTS;
dist/if/if.pm​:88​: no if $] ge 5.021_006, warnings => "locale";
dist/if/t/if.t​:38​:is(eval "no if ($v_plus > \$])", undef,
dist/if/t/if.t​:39​: "Too few args to 'no if' returns <undef>");
dist/if/t/if.t​:40​:like($@​, qr/Too few arguments to 'no if'/, " ... and returns correct error");
cpan/autodie/t/exceptions.t​:12​:no if $] >= 5.017011, warnings => "experimental​::smartmatch";
cpan/autodie/lib/Fatal.pm​:1060​: no if \$\] >= 5.017011, warnings => "experimental​::smartmatch";
pod/perl5180delta.pod​:44​: no if $] >= 5.018, warnings => "experimental​::feature_name";
pod/perl5180delta.pod​:408​: no if $] >= 5.018, warnings => "experimental​::smartmatch";
pod/perl5200delta.pod​:1537​: no if $] >= 5.01908, warnings => "experimental​::autoderef";
t/comp/parser.t​:451​:eval 'no if $] >= 5.17.4 warnings => "deprecated"';

@p5pRT
Copy link
Author

@p5pRT p5pRT commented Jan 25, 2018

From @Grinnz

On Wed, Jan 24, 2018 at 4​:18 PM, James E Keenan via RT <
perlbug-followup@​perl.org> wrote​:

On Wed, 24 Jan 2018 17​:38​:14 GMT, grinnz@​gmail.com wrote​:

That should be reasonable, but note that not many modules implement
unimport; 'no warnings' is probably the most common usage of 'no'.

-Dan

There are many instances of the 'no' function in the source code, but very
few instances of the string 'no if'. See attachment for output of 'ack
'\bno\s+if\b' .'. Almost every instance is testing one of Perl's special
variables against some value and, as you say, most are warnings-related.

Unfortunately that isn't getting us any close to solving the problem. Is
anyone familiar with where/how 'no' is implemented in the source code?

'no' is the same as 'use' but it calls unimport instead of import. The
complications just come from modules not generally implementing an unimport
method that mirrors the import (mostly because unimporting functions
doesn't really make practical sense). So only pragmas that create lexical
effects tend to use it.

-Dan

@p5pRT
Copy link
Author

@p5pRT p5pRT commented Jan 25, 2018

From @sisyphus

-----Original Message-----
From​: James E Keenan via RT
Sent​: Thursday, January 25, 2018 8​:18 AM
To​: OtherRecipients of perl Ticket #132732​:
Cc​: perl5-porters@​perl.org
Subject​: [perl #132732] use if - behaviour does not match documentation

Is anyone familiar with where/how 'no' is implemented in the source code?

I'm thinking 3 distinct cases​:

1) You want to load a module unconditionally ('use MODULE;')

2) You want to load a module conditionally ('use if CONDITION, MODULE,
FUNCTIONS;')

3) Having loaded a module, you then wish to conditionally remove specific
piece of that module's functionality ('no if CONDITION, MODULE,
EXCLUSIONS').

As an example, 'no if' seems to work fine with warnings.pm​:

### 1 ###
use warnings;
no if 0, warnings => "numeric";
print "$]\n";
print "hello world" + 1, "\n";
print "\$z​: $z\n";
########

Outputs​:

Name "main​::z" used only once​: possible typo at try.pl line 5.
5.027008
Argument "hello world" isn't numeric in addition (+) at try.pl line 4.
1
Use of uninitialized value $z in concatenation (.) or string at try.pl line
5.
$z​:

If we change the condition to 'no if 1', then the output is the same, except
that the numeric warning has disappeared.
And that's as to be expected.

Similarly, with strict.pm, the following works fine, outputting "DONE"​:

use strict;
no if 1, strict => "subs";
use if 1, Math​::BigInt => 1;
print "DONE\n";

That would blow up at compile time if strict "subs" was enabled - which is
exactly what happens if the condition is changed to 'no if 0'.

But the same doesn't happen with re, as James has discovered.

use re;
no if 1, re => "is_regexp";
is_regexp(1);
print "DONE\n";

simply outputs "DONE" instead of throwing an error.

Is the difference that (wrt re.pm), we're actually explicitly calling a
function - whereas, with warnings, "numeric" is not a function that we
explicitly call, just as "subs" is not a strict.pm function that we
explicitly call ??

Cheers,
Rob

@p5pRT
Copy link
Author

@p5pRT p5pRT commented Jan 25, 2018

From @Grinnz

On Wed, Jan 24, 2018 at 8​:42 PM, <sisyphus1@​optusnet.com.au> wrote​:

Is the difference that (wrt re.pm), we're actually explicitly calling a
function - whereas, with warnings, "numeric" is not a function that we
explicitly call, just as "subs" is not a strict.pm function that we
explicitly call ??

As I explained before, the difference is that the 'unimport' method for
re.pm does not unimport subroutines, because that's impractical.

I believe the question was about implementation of 'no' itself, but for use
cases, there are also several pragmas that are useful via 'no' even if they
weren't already activated with 'use', usually because it reads better that
way, for example​:

no autovivification;
no bareword​::filehandles;

So one could conceive of a case where you'd want to conditionally
"activate" one of these pragmas.

-Dan

@p5pRT
Copy link
Author

@p5pRT p5pRT commented Jan 25, 2018

From @Abigail

On Wed, Jan 24, 2018 at 10​:09​:56AM -0800, James E Keenan via RT wrote​:

On Wed, 24 Jan 2018 17​:38​:14 GMT, grinnz@​gmail.com wrote​:

On Wed, Jan 24, 2018 at 12​:33 PM, James E Keenan <jkeenan@​pobox.com>
wrote​:

On 01/24/2018 12​:20 PM, Dan Book wrote​:

From the source code, 'no if' appears to do the same thing as 'use
if'
but runs unimport instead of import. It's not a negation of the
condition.
This is consistent with 'no' and 'use' but the negation is not
explained
very well IMO.

So, would a proper test then be to import specific functions, then
use 'no
if CONDITION, "MODULE" ARGUMENTS to unimport them?

That should be reasonable, but note that not many modules implement
unimport; 'no warnings' is probably the most common usage of 'no'.

-Dan

Well, the reason I used bigrat in the tests is that it does contain a 'sub unimport'.

As does 're'. But there I get even stranger results. Try this out (in the patch)​:

#####
{
eval "use if (0 > 1), q|re|, qw(is_regexp regexp_pattern);";
ok (! re->can('is_regexp'), "Cannot is_regexp");
ok (! re->can('regexp_pattern'), "Cannot regexp_pattern");
ok ( re->can('is_regexp'), "Can is_regexp");
ok ( re->can('regexp_pattern'), "Can regexp_pattern");
}
#####
I get​:
#####
not ok 11 - Cannot is_regexp
# Failed test 'Cannot is_regexp'
# at t/if.t line 73.
not ok 12 - Cannot regexp_pattern
# Failed test 'Cannot regexp_pattern'
# at t/if.t line 74.
ok 13 - Can is_regexp
ok 14 - Can regexp_pattern
#####

Which I read as, "Whether or not the CONDITION evaluates to true or not, the two functions are imported from package 're'."

That code checks whether the class 're' has a subroutine 'is_regexp' --
which it will as long as it's loaded.

To check whether it has been imported, you need to call "can" on the
class functions are imported *to*​:

  #####
  {
  eval "use if (0 > 1), q|re|, qw(is_regexp regexp_pattern);";
  ok (! main​::->can('is_regexp'), "Cannot is_regexp");
  ok (! main​::->can('regexp_pattern'), "Cannot regexp_pattern");
  ok ( main​::->can('is_regexp'), "Can is_regexp");
  ok ( main​::->can('regexp_pattern'), "Can regexp_pattern");
  }
  #####

Assuming your code runs in the main package.

Abigail

@p5pRT
Copy link
Author

@p5pRT p5pRT commented Jan 26, 2018

From @jkeenan

On Thu, 25 Jan 2018 11​:29​:38 GMT, abigail@​abigail.be wrote​:

On Wed, Jan 24, 2018 at 10​:09​:56AM -0800, James E Keenan via RT wrote​:

On Wed, 24 Jan 2018 17​:38​:14 GMT, grinnz@​gmail.com wrote​:

On Wed, Jan 24, 2018 at 12​:33 PM, James E Keenan
<jkeenan@​pobox.com>
wrote​:

On 01/24/2018 12​:20 PM, Dan Book wrote​:

From the source code, 'no if' appears to do the same thing as
'use
if'
but runs unimport instead of import. It's not a negation of the
condition.
This is consistent with 'no' and 'use' but the negation is not
explained
very well IMO.

So, would a proper test then be to import specific functions,
then
use 'no
if CONDITION, "MODULE" ARGUMENTS to unimport them?

That should be reasonable, but note that not many modules implement
unimport; 'no warnings' is probably the most common usage of 'no'.

-Dan

Well, the reason I used bigrat in the tests is that it does contain a
'sub unimport'.

As does 're'. But there I get even stranger results. Try this out
(in the patch)​:

#####
{
eval "use if (0 > 1), q|re|, qw(is_regexp regexp_pattern);";
ok (! re->can('is_regexp'), "Cannot is_regexp");
ok (! re->can('regexp_pattern'), "Cannot regexp_pattern");
ok ( re->can('is_regexp'), "Can is_regexp");
ok ( re->can('regexp_pattern'), "Can regexp_pattern");
}
#####
I get​:
#####
not ok 11 - Cannot is_regexp
# Failed test 'Cannot is_regexp'
# at t/if.t line 73.
not ok 12 - Cannot regexp_pattern
# Failed test 'Cannot regexp_pattern'
# at t/if.t line 74.
ok 13 - Can is_regexp
ok 14 - Can regexp_pattern
#####

Which I read as, "Whether or not the CONDITION evaluates to true or
not, the two functions are imported from package 're'."

That code checks whether the class 're' has a subroutine 'is_regexp'
--
which it will as long as it's loaded.

To check whether it has been imported, you need to call "can" on the
class functions are imported *to*​:

#####
{
eval "use if (0 > 1), q|re|, qw(is_regexp regexp_pattern);";
ok (! main​::->can('is_regexp'), "Cannot is_regexp");
ok (! main​::->can('regexp_pattern'), "Cannot regexp_pattern");
ok ( main​::->can('is_regexp'), "Can is_regexp");
ok ( main​::->can('regexp_pattern'), "Can regexp_pattern");
}
#####

Assuming your code runs in the main package.

Abigail

Thanks for the suggestion. I believe I now have better documentation and testing of 'if'. Please review the new patch attached, 132732-0001-if-module-clarify-documentation-and-test-more-thorou.patch.

Thank you very much.

--
James E Keenan (jkeenan@​cpan.org)

@p5pRT
Copy link
Author

@p5pRT p5pRT commented Jan 26, 2018

From @jkeenan

132732-0001-if-module-clarify-documentation-and-test-more-thorou.patch
From e202bfb954217349f5e61bddbb92ba10021ea242 Mon Sep 17 00:00:00 2001
From: James E Keenan <jkeenan@cpan.org>
Date: Tue, 23 Jan 2018 10:46:32 -0500
Subject: [PATCH] 'if' module:  clarify documentation and test more thoroughly.

The documentation for 'if' made certain claims about the need to quote or not
quote a module name preceding a "fat arrow" ('=>') operator.  These claims
were shown to be unfounded in most cases when "use strict 'subs'" was in
effect.

In the course of writing better documentation, it was observed that the "no
if" case was very under-tested, poorly documented and hence poorly understood.
Hence, more tests have been added and the documentation has been extensively
revised.  However, there have been no changes in source code or functionality.

Make porting/podcheck.t happy.  Compensate for functions not available on
older perls.

For:  RT # 132732.
---
 dist/if/if.pm  |  84 ++++++++++++++++++++++--------------------------
 dist/if/t/if.t | 100 ++++++++++++++++++++++++++++++++++++++++++++-------------
 2 files changed, 117 insertions(+), 67 deletions(-)

diff --git a/dist/if/if.pm b/dist/if/if.pm
index d1cbd00..23ba642 100644
--- a/dist/if/if.pm
+++ b/dist/if/if.pm
@@ -1,6 +1,6 @@
 package if;
 
-$VERSION = '0.0607';
+$VERSION = '0.0608';
 
 sub work {
   my $method = shift() ? 'import' : 'unimport';
@@ -25,76 +25,70 @@ __END__
 
 =head1 NAME
 
-if - C<use> a Perl module if a condition holds (also can C<no> a module)
+if - C<use> a Perl module if a condition holds
 
 =head1 SYNOPSIS
 
-  use if CONDITION, MODULE => ARGUMENTS;
-  no if CONDITION, MODULE => ARGUMENTS;
+    use if CONDITION, "MODULE", ARGUMENTS;
+    no  if CONDITION, "MODULE", ARGUMENTS;
 
 =head1 DESCRIPTION
 
-The C<if> module is used to conditionally load or unload another module.
-The construct
+=head2 C<use if>
 
-  use if CONDITION, MODULE => ARGUMENTS;
+The C<if> module is used to conditionally load another module.  The construct:
 
-will load MODULE only if CONDITION evaluates to true.
-The above statement has no effect unless C<CONDITION> is true.
-If the CONDITION does evaluate to true, then the above line has
-the same effect as:
+    use if CONDITION, "MODULE", ARGUMENTS;
 
-  use MODULE ARGUMENTS;
+... will load C<MODULE> only if C<CONDITION> evaluates to true; it has no
+effect if C<CONDITION> evaluates to false.  (The module name, assuming it
+contains at least one C<::>, must be quoted when C<'use strict "subs";'> is in
+effect.)  If the CONDITION does evaluate to true, then the above line has the
+same effect as:
 
-The use of C<< => >> above provides necessary quoting of C<MODULE>.
-If you don't use the fat comma (eg you don't have any ARGUMENTS),
-then you'll need to quote the MODULE.
+    use MODULE ARGUMENTS;
 
-If you wanted ARGUMENTS to be an empty list, i.e. have the effect of:
+For example, the F<Unicode::UCD> module's F<charinfo> function will use two functions from F<Unicode::Normalize> only if a certain condition is met:
+
+    use if defined &DynaLoader::boot_DynaLoader,
+        "Unicode::Normalize" => qw(getCombinClass NFD);
+
+Suppose you wanted C<ARGUMENTS> to be an empty list, I<i.e.>, to have the
+effect of:
 
     use MODULE ();
 
-you can't do this with the C<if> pragma; however, you can achieve
+You can't do this with the C<if> pragma; however, you can achieve
 exactly this effect, at compile time, with:
 
     BEGIN { require MODULE if CONDITION }
 
-=head2 EXAMPLES
-
-The following line is taken from the testsuite for L<File::Map>:
-
-  use if $^O ne 'MSWin32', POSIX => qw/setlocale LC_ALL/;
-
-If run on any operating system other than Windows,
-this will import the functions C<setlocale> and C<LC_ALL> from L<POSIX>.
-On Windows it does nothing.
-
-The following is used to L<deprecate> core modules beyond a certain version of Perl:
+=head2 C<no if>
 
-  use if $] > 5.016, 'deprecate';
+The C<no if> construct is mainly used to deactivate categories of warnings
+when those categories would produce superfluous output under specified
+versions of F<perl>.
 
-This line is taken from L<Text::Soundex> 3.04,
-and marks it as deprecated beyond Perl 5.16.
-If you C<use Text::Soundex> in Perl 5.18, for example,
-and you have used L<warnings>,
-then you'll get a warning message
-(the deprecate module looks to see whether the
-calling module was C<use>'d from a core library directory,
-and if so, generates a warning),
-unless you've installed a more recent version of L<Text::Soundex> from CPAN.
+For example, the C<redundant> category of warnings was introduced in
+Perl-5.22.  This warning flags certain instances of superfluous arguments to
+C<printf> and C<sprintf>.  But if your code was running warnings-free on
+earlier versions of F<perl> and you don't care about C<redundant> warnings in
+more recent versions, you can call:
 
-You can also specify to NOT use something:
+    use warnings;
+    no if $] >= 5.022, q|warnings|, qw(redundant);
 
- no if $] ge 5.021_006, warnings => "locale";
+    my $test    = { fmt  => "%s", args => [ qw( x y ) ] };
+    my $result  = sprintf $test->{fmt}, @{$test->{args}};
 
-This warning category was added in the specified Perl version (a development
-release).  Without the C<'if'>, trying to use it in an earlier release would
-generate an unknown warning category error.
+The C<no if> construct assumes that a module or pragma has correctly
+implemented an C<unimport()> method -- but most modules and pragmata have not.
+That explains why the C<no if> construct is of limited applicability.
 
 =head1 BUGS
 
-The current implementation does not allow specification of the
-required version of the module.
+The current implementation does not allow specification of the required
+version of the module.
 
 =head1 SEE ALSO
 
diff --git a/dist/if/t/if.t b/dist/if/t/if.t
index 4a2b351..827d93c 100644
--- a/dist/if/t/if.t
+++ b/dist/if/t/if.t
@@ -1,9 +1,9 @@
 #!./perl
 
 use strict;
-use Test::More tests => 10;
+use Test::More tests => 18;
 
-my $v_plus = $] + 1;
+my $v_plus  = $] + 1;
 my $v_minus = $] - 1;
 
 unless (eval 'use open ":std"; 1') {
@@ -12,29 +12,85 @@ unless (eval 'use open ":std"; 1') {
   eval 'sub open::foo{}';		# Just in case...
 }
 
-no strict;
+{
+    no strict;
 
-is( eval "use if ($v_minus > \$]), strict => 'subs'; \${'f'} = 12", 12,
-    '"use if" with a false condition, fake pragma');
-is( eval "use if ($v_minus > \$]), strict => 'refs'; \${'f'} = 12", 12,
-    '"use if" with a false condition and a pragma');
+    is( eval "use if ($v_minus > \$]), strict => 'subs'; \${'f'} = 12", 12,
+        '"use if" with a false condition, fake pragma');
+    is( eval "use if ($v_minus > \$]), strict => 'refs'; \${'f'} = 12", 12,
+        '"use if" with a false condition and a pragma');
 
-is( eval "use if ($v_plus > \$]), strict => 'subs'; \${'f'} = 12", 12,
-    '"use if" with a true condition, fake pragma');
+    is( eval "use if ($v_plus > \$]), strict => 'subs'; \${'f'} = 12", 12,
+        '"use if" with a true condition, fake pragma');
 
-is( eval "use if ($v_plus > \$]), strict => 'refs'; \${'f'} = 12", undef,
-    '"use if" with a true condition and a pragma');
-like( $@, qr/while "strict refs" in use/, 'expected error message'),
+    is( eval "use if ($v_plus > \$]), strict => 'refs'; \${'f'} = 12", undef,
+        '"use if" with a true condition and a pragma');
+    like( $@, qr/while "strict refs" in use/, 'expected error message'),
 
-# Old version had problems with the module name 'open', which is a keyword too
-# Use 'open' =>, since pre-5.6.0 could interpret differently
-is( (eval "use if ($v_plus > \$]), 'open' => IN => ':crlf'; 12" || 0), 12,
-    '"use if" with open');
+    # Old version had problems with the module name 'open', which is a keyword too
+    # Use 'open' =>, since pre-5.6.0 could interpret differently
+    is( (eval "use if ($v_plus > \$]), 'open' => IN => ':crlf'; 12" || 0), 12,
+        '"use if" with open');
 
-is(eval "use if ($v_plus > \$])", undef,
-   "Too few args to 'use if' returns <undef>");
-like($@, qr/Too few arguments to 'use if'/, "  ... and returns correct error");
+    is(eval "use if ($v_plus > \$])", undef,
+       "Too few args to 'use if' returns <undef>");
+    like($@, qr/Too few arguments to 'use if'/, "  ... and returns correct error");
 
-is(eval "no if ($v_plus > \$])", undef,
-   "Too few args to 'no if' returns <undef>");
-like($@, qr/Too few arguments to 'no if'/, "  ... and returns correct error");
+    is(eval "no if ($v_plus > \$])", undef,
+       "Too few args to 'no if' returns <undef>");
+    like($@, qr/Too few arguments to 'no if'/, "  ... and returns correct error");
+}
+
+{
+    note(q|RT 132732: strict 'subs'|);
+    use strict "subs";
+
+    {
+        SKIP: {
+            unless ($] >= 5.018) {
+                skip "bigrat apparently not testable prior to perl-5.18", 4;
+            }
+            note(q|strict "subs" : 'use if' : condition false|);
+            eval "use if (0 > 1), q|bigrat|, qw(hex oct);";
+            ok (! main->can('hex'), "Cannot call bigrat::hex() in importing package");
+            ok (! main->can('oct'), "Cannot call bigrat::oct() in importing package");
+
+            note(q|strict "subs" : 'use if' : condition true|);
+            eval "use if (1 > 0), q|bigrat|, qw(hex oct);";
+            ok (  main->can('hex'), "Can call bigrat::hex() in importing package");
+            ok (  main->can('oct'), "Can call bigrat::oct() in importing package");
+        }
+    }
+
+    {
+        note(q|strict "subs" : 'no if' : condition variable|);
+        note(($] >= 5.022) ? "Recent enough Perl: $]" : "Older Perl: $]");
+        use warnings;
+        SKIP: {
+            unless ($] >= 5.022) {
+                skip "Redundant argument warning not available in pre-5.22 perls", 4;
+            }
+
+            {
+                no if $] >= 5.022, q|warnings|, qw(redundant);
+                my ($test, $result, $warn);
+                local $SIG{__WARN__} = sub { $warn = shift };
+                $test = { fmt  => "%s", args => [ qw( x y ) ] };
+                $result = sprintf $test->{fmt}, @{$test->{args}};
+                is($result, $test->{args}->[0], "Got expected string");
+                ok(! $warn, "Redundant argument warning suppressed");
+            }
+
+            {
+                use if $] >= 5.022, q|warnings|, qw(redundant);
+                my ($test, $result, $warn);
+                local $SIG{__WARN__} = sub { $warn = shift };
+                $test = { fmt  => "%s", args => [ qw( x y ) ] };
+                $result = sprintf $test->{fmt}, @{$test->{args}};
+                is($result, $test->{args}->[0], "Got expected string");
+                like($warn, qr/Redundant argument in sprintf/,
+                    "Redundant argument warning generated and capture");
+            }
+        }
+    }
+}
-- 
2.7.4

@p5pRT
Copy link
Author

@p5pRT p5pRT commented Jan 29, 2018

From @jkeenan

On Fri, 26 Jan 2018 02​:07​:55 GMT, jkeenan wrote​:

Thanks for the suggestion. I believe I now have better documentation
and testing of 'if'. Please review the new patch attached, 132732-
0001-if-module-clarify-documentation-and-test-more-thorou.patch.

Thank you very much.

I plan to apply the patch on Thu, Feb 01 unless there is objection.

Thank you very much.
--
James E Keenan (jkeenan@​cpan.org)

@p5pRT
Copy link
Author

@p5pRT p5pRT commented Jan 30, 2018

From @sisyphus

-----Original Message-----
From​: James E Keenan via RT
Sent​: Tuesday, January 30, 2018 10​:10 AM
To​: OtherRecipients of perl Ticket #132732​:
Cc​: perl5-porters@​perl.org
Subject​: [perl #132732] use if - behaviour does not match documentation

.....

Please review the new patch attached, 132732-
0001-if-module-clarify-documentation-and-test-more-thorou.patch.

I plan to apply the patch on Thu, Feb 01 unless there is objection.

Jim,

That all looks pretty good to me, and tests fine.

The last paragraph in the "SEE ALSO" section of the POD (which pre-dates
your rewrite) threw me for a few seconds. My perldoc utility renders it
simply as​:

provide can be used to select one of several possible modules to load,
based on what version of Perl is running.

Probably not so confusing if you know that there's a module named
"provide" - but I didn't know that, and it's not part of core.

Maybe something like​:
=~ s/L<provide>/The L<provide> module from CPAN/

Also, the end of the sentence ("based on what version of Perl is running")
makes me grimace a bit, as do the replacements that I come up with - namely,
"based on the version of Perl that is running" or just "based on the version
of Perl".
So I'm not going to make *any* suggestions about that.

Thanks for being thorough ... and for doing all the work.

Cheers,
Rob

@p5pRT
Copy link
Author

@p5pRT p5pRT commented Feb 1, 2018

From @jkeenan

On Tue, 30 Jan 2018 04​:25​:40 GMT, sisyphus wrote​:

-----Original Message-----
From​: James E Keenan via RT
Sent​: Tuesday, January 30, 2018 10​:10 AM
To​: OtherRecipients of perl Ticket #132732​:
Cc​: perl5-porters@​perl.org
Subject​: [perl #132732] use if - behaviour does not match
documentation

.....

Please review the new patch attached, 132732-
0001-if-module-clarify-documentation-and-test-more-thorou.patch.

I plan to apply the patch on Thu, Feb 01 unless there is objection.

Jim,

That all looks pretty good to me, and tests fine.

The last paragraph in the "SEE ALSO" section of the POD (which pre-
dates
your rewrite) threw me for a few seconds. My perldoc utility renders
it
simply as​:

provide can be used to select one of several possible modules to load,
based on what version of Perl is running.

Probably not so confusing if you know that there's a module named
"provide" - but I didn't know that, and it's not part of core.

Maybe something like​:
=~ s/L<provide>/The L<provide> module from CPAN/

Also, the end of the sentence ("based on what version of Perl is
running")
makes me grimace a bit, as do the replacements that I come up with -
namely,
"based on the version of Perl that is running" or just "based on the
version
of Perl".
So I'm not going to make *any* suggestions about that.

Thanks for being thorough ... and for doing all the work.

Cheers,
Rob

Pushed to blead in 1654584 with some documentation touch-ups as suggested by Rob.

Thank you very much.

--
James E Keenan (jkeenan@​cpan.org)

@p5pRT
Copy link
Author

@p5pRT p5pRT commented Feb 1, 2018

@jkeenan - Status changed from 'open' to 'pending release'

@p5pRT
Copy link
Author

@p5pRT p5pRT commented Jun 23, 2018

From @khwilliamson

Thank you for filing this report. You have helped make Perl better.

With the release yesterday of Perl 5.28.0, this and 185 other issues have been
resolved.

Perl 5.28.0 may be downloaded via​:
https://metacpan.org/release/XSAWYERX/perl-5.28.0

If you find that the problem persists, feel free to reopen this ticket.

@p5pRT p5pRT closed this as completed Jun 23, 2018
@p5pRT
Copy link
Author

@p5pRT p5pRT commented Jun 23, 2018

@khwilliamson - Status changed from 'pending release' to 'resolved'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant