oshun icon indicating copy to clipboard operation
oshun copied to clipboard

Data::Checks `import` and `unimport` should leave strict and warnings the hell alone

Open tobyink opened this issue 1 year ago • 6 comments

https://github.com/Perl-Oshun/oshun/blob/66eb75e20079881d9fb96ef774ef2e69858c911b/lib/Data/Checks.pm#L30

Consider:

use strict;
use warnings;
use Data::Checks;

{
   no Data::Checks; # Turn off Data::Checks in this scope
   ...;             # but strict and warnings have also been disabled!
}

tobyink avatar Jul 25 '23 15:07 tobyink

@tobyink how it should behave with:

no strict;
no warnings;
use Data::Check;

{
    no Data::Check;
   # should there be warnings / strict enabled?
}

happy-barney avatar Jul 25 '23 15:07 happy-barney

use Data::Checks shouldn't enable or disable strict or warnings.

no Data::Checks shouldn't enable or disable strict or warnings.

In answer to your example:

no strict;
no warnings;
use Data::Checks;

{
    no Data::Checks;
    # There should be no strict or warnings here, because
    # the code above says `no strict` and `no warnings`.
}

Enabling signatures, I'm okay with, because Data::Checks necessarily piggybacks on that syntax.

tobyink avatar Jul 25 '23 15:07 tobyink

thanks for clarification (I cannot agree more) ... only wish for Perl itself to allow current implementation and proper unimport

happy-barney avatar Jul 25 '23 16:07 happy-barney

@tobyink I'm sorry I didn't respond sooner. I've had other issues I've had to deal with.

You say that Data::Checks should not touch strict and warnings, but you didn't explain why. The reason I enabled them was simple: imagine these getting into core for 5.42 and use v5.42.0 automatically enables checks. It also automatically enables strict and warnings. Thus, I was trying to mirror behavior we'd likely have. Turning off strict and warnings is trivial.

The point of checks is to make it easier to write correct code and disabling strict (and to a lesser extent, warnings) goes against the point of the Oshun project. So I can see you feel strongly about this, but I don't understand why, especially since it's so trivial to disable the safety.

Ovid avatar Jul 31 '23 07:07 Ovid

I thought the example I provided made things clear why.

use strict;
use warnings;
use Data::Checks;

{
   no Data::Checks;
   
   # In this scope, strict and warnings are DISABLED!
   # I never asked for them to be disabled.
   # Data::Checks->unimport disabled them for me!
   # This is incredibly unintuitive.
}

As you mentioned, use v5.42.0 will already enable strict and warnings. (Indeed, use v5.38 already does!) so there's literally no need for Data::Checks to touch them.

tobyink avatar Jul 31 '23 08:07 tobyink

(Deleted previous comment because I had names backwards)

@tobyink I see your point now. You're absolutely right. If you or someone else would like to provide a PR, I'd be happy to review!

Ovid avatar Jul 31 '23 09:07 Ovid