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

Show problem with vars_ok() re lexicals declared outside sub def #59

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

Conversation

jkeenan
Copy link
Collaborator

@jkeenan jkeenan commented Sep 13, 2024

NOTE: This patch is for research; it's not ready to be committed to master.

In #44, it was argued that vars_ok() should have failed on a .pm file that simply declared three 'my' variables but never used them and never did anything else. vars_ok() returns '1' (as do the previous invocations of vars_ok() in the test file) where we would have expected it to return a Perl-false value.

This patch attempts to bring that observation into the test suite in the form of a TODO test. However, we're hampered by the fact that the return value of vars_ok() is at best opaquely documented. What we really need is a function along the lines of:

vars_not_expected_to_be_ok('path/to/file');

NOTE: This patch is for research; it's not ready to be committed to
master.

In houseabsolute#44, it was
argued that vars_ok() should have failed on a .pm file that simply
declared three 'my' variables but never used them and never did anything
else.  vars_ok() returns '1' (as do the previous invocations of
vars_ok() in the test file) where we would have expected it to return a
Perl-false value.

This patch attempts to bring that observation into the test suite in the
form of a TODO test.  However, we're hampered by the fact that the
return value of vars_ok() is at best opaquely documented.  What we
really need is a function along the lines of:

    vars_not_expected_to_be_ok('path/to/file');
@jkeenan jkeenan marked this pull request as draft September 13, 2024 13:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

1 participant