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

Set $^W default to off. #427

Closed
wants to merge 1 commit into from
Closed

Set $^W default to off. #427

wants to merge 1 commit into from

Conversation

ambs
Copy link
Member

@ambs ambs commented Sep 3, 2013

Requested by Matt Trout. Probably he can argue here why, or even suggest further changes.

@shadowcat-mst
Copy link
Contributor

$^W forces warnings on for everything.

The entire perl5 VM.

Not just your code.

This means that code that was intentionally written without 'use warnings;' can end up having them forced on.

If you want to actually import warnings into a module when somebody does 'use Dancer', warnings->import::into($module) will work.

@veryrusty
Copy link
Member

I'd prefer Dancer2 be opinionated enough that when you use Dancer2 you get warnings imported into that module.

These days Dancer1 does that and has a global_warnings config option (see PerlDancer/Dancer@59f6b4c).

My two cents: Dancer2->import does a warnings->import (along with strict and utf8). Be drastic and totally purge the import_warnings config option (and its trigger that sets $^W). I would not include the global_warnings config option Dancer1 has; if someone wants it, they can set $^W in their app.psgi file and deal with the consequences.

@ambs - I'm happy to work on an alternate pull request since I'm dissing this one..!

@ambs
Copy link
Member Author

ambs commented Sep 7, 2013

@veryrusty, this was a quick hack to answer to @shadowcat-mst on IRC. I am happy with different approaches. Please go ahead :-)

@shadowcat-mst
Copy link
Contributor

On Sat, Sep 07, 2013 at 05:40:27AM -0700, Alberto Simões wrote:

@veryrusty, this was a quick hack to answer to @shadowcat-mst on IRC. I am happy with different approaches. Please go ahead :-)

I would -love- to see the $^W thing die in a -ing fire. Covered in bees.

I would also love to see warnings imported along with strict (given you're
using Moo you might want to consider applying strictures instead for even
more safety).

But mostly I just wanted us to stop screwing people by turning it on, and
when I had this argument over Dancer1 that was as far as I'd managed to get
with the argument.

If you're willing to put together a PR that fixes this properly, I will be
all in favour.

Matt S Trout - Shadowcat Systems - Perl consulting with a commit bit and a clue

http://shadowcat.co.uk/blog/matt-s-trout/ http://twitter.com/shadowcat_mst/

Email me now on mst (at) shadowcat.co.uk and let's chat about how our CPAN
commercial support, training and consultancy packages could help your team.

veryrusty added a commit to veryrusty/Dancer2 that referenced this pull request Sep 8, 2013
…setting.

Use warnings->import when importing Dancer2 to ensure any code using it has
the warnings pragma enabled (just like strict and utf8).

L<perllexwarn> states ".. using $^W to either disable or enable blocks of
code is fundamentally flawed.". Also see Matt Trout's comments in PerlDancer#427.

Remove the import_config setting and its trigger that mucks with $^W and purge
it from the POD. If anyone really wants to set $^W, they can add it to their
app or psgi file and be in control of the results.
@xsawyerx
Copy link
Member

xsawyerx commented Sep 8, 2013

I agree with the decision to remove global warnings, but we should still import warnings to the caller, per @shadowcat-mst's example.

It seems @veryrusty addressed it in #443 after the fruitful discussion this PR has provided, so I'm closing it.

Thanks everyone. :)

@xsawyerx xsawyerx closed this Sep 8, 2013
@ambs
Copy link
Member Author

ambs commented Sep 8, 2013

@veryrusty++ as usual.

@xsawyerx xsawyerx deleted the change/import_warnings branch September 18, 2014 11:28
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

Successfully merging this pull request may close these issues.

4 participants