Skip to content

Clarify that __clean_eval does not use strict or warnings. #35

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Clarify that __clean_eval does not use strict or warnings. #35

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Changes
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
Revision history for Module-Metadata

{{$NEXT}}
- Clarify that __clean_eval does not include strict or warnings

1.000037 2019-09-07 18:32:44Z
- add decode_pod option for automatic =encoding handling
Expand Down
2 changes: 1 addition & 1 deletion lib/Module/Metadata.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ package Module::Metadata;
# perl modules (assuming this may be expanded in the distant
# parrot future to look at other types of modules).

sub __clean_eval { eval $_[0] }
sub __clean_eval { no strict; no warnings; eval $_[0] }
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm mostly ok with this, but just would like to note that "no warnings" does not currently bring things to the "default" state, it in fact disables default warnings. However, I don't believe there is any reason for this module to care about default warnings here either.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a specific warning category that is being targeted here? If so, can't we just disable that one?

In other words, what forms of syntax are being tripped up by having warnings enabled?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is meant to guard against anything specific, but against the possibility of warnings being enabled in an outer scope.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was simply trying to explicitly do what I guessed the intentions were by declaring this sub prior to use warnings/strict.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And as far as I know there is no way to declare that a scope should use what is currently the default set of warnings, which is an unfortunate limitation of the warnings.pm api.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO the current change make the original intent clear.

The function __clean_eval as it stands (before this fix) is eval without warnings and strict hint set.
With this patch, it stays the same but would avoid someone to figure out what's happening and make the intent clear.

Yes this is hiding some issues that would have been exposed with strict and warnings, but I think this is a different case, no?

That code runs without warnings and strict being set.

sub alien { $x = "2:" + 3; return $x }

use strict;
use warnings;

print alien() . "\n";

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct. I am pointing out that it is not in fact the same as without warnings and strict set. "no warnings" also disables default warnings.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct. I am pointing out that it is not in fact the same as without warnings and strict set. "no warnings" also disables default warnings.

What do you suggest instead?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would note that this sub is only used in 2 places and the change that created it seemed to want to turn off all strict/warnings regardless. I didn't write it so I can only speculate on the intention.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I had a better suggestion I would not have approved it 😉

use strict;
use warnings;

Expand Down