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

Jkeenan/warnings t base #287

Merged
merged 4 commits into from
Oct 10, 2020
Merged

Conversation

jkeenan
Copy link
Collaborator

@jkeenan jkeenan commented Sep 28, 2020

Reviewers: There's one aspect of this pull request which warrants some discussion.

perldoc perlhack advises:

   *   t/base, t/comp and t/opbasic

        Since we don't know if "require" works, or even subroutines, use ad
        hoc tests for these three. Step carefully to avoid using the feature
        being tested. Tests in t/opbasic, for instance, have been placed
        there rather than in t/op because they test functionality which
        t/test.pl presumes has already been demonstrated to work.

For these 3 subdirectories -- and for t/base in particular -- we have tended to write tests on the premise that require and use have not yet been demonstrated to work. But if use MODULE has not yet been proven to work, then presumably no MODULE has also not yet been proven to work.

However, once we run test files in these directories under strict-by-default and/or warnings-by-default, we have to have a way of temporarily relaxing strictures or suppressing warnings. That suggests judicious, precise use of, say, no strict 'uninitialized;' and no strict 'refs;'. But that presumes that we can load modules.

We don't face this problem in Perl 5 because most files in these subdirectories never cared about strictures or warnings. Indeed, some of them date to Perl 1 in 1987 -- long before strictures and warnings even existed.

See, for example, t/base/lex.t (commit d4f1572, already in alpha) and t/base/num.t (commit 3a4795b, in this pull request).

How shall we square this circle?

Thank you very much.
Jim Keenan

@jkeenan jkeenan added question Further information is requested or policy needs discussion warnings-by-default labels Sep 28, 2020
@jkeenan jkeenan self-assigned this Sep 28, 2020
@xsawyerx
Copy link
Collaborator

no warnings and no strict affect variables. Could we not edit these variables directly? Then the only assumptions we have are 1. That the ops for setting these variables work, 2. That variables work in general, 3. That those variables work in particular. These might be much easier to live with than the assumptions that subroutines work.

t/base/lex.t Show resolved Hide resolved
print "foo\n";
}
/^/ && (print "ok 5\n");
'; # '

my %foo;
eval '$foo{1} / 1;';
eval 'no warnings q|uninitialized|; $foo{1} / 1;';
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm unclear if doing this outside of blocks has a global effect.

Copy link
Owner

Choose a reason for hiding this comment

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

alternatively we can use something like this BEGIN { ${^WARNING_BITS} = '' }

t/base/term.t Outdated Show resolved Hide resolved
@jkeenan
Copy link
Collaborator Author

jkeenan commented Sep 28, 2020

no warnings and no strict affect variables. Could we not edit these variables directly?

@xsawyerx, Can you clarify what you mean by this? Perhaps by giving an example?

For instance, how would you address the warnings generated by running t/base/num.t in the alpha-dev-03-warnings branch?

Thank you very much.
Jim Keenan

Copy link
Owner

@atoomic atoomic left a comment

Choose a reason for hiding this comment

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

This looks ok, but I would consider getting rid of the bare file handle and use something like this instead

my $fh;
open($fh, "/dev/null") or open($fh, "nla0:") or (die "Can't open /dev/null.");`
....

t/base/term.t Show resolved Hide resolved
t/base/num.t Show resolved Hide resolved
@jkeenan
Copy link
Collaborator Author

jkeenan commented Sep 29, 2020

This looks ok, but I would consider getting rid of the bare file handle and use something like this instead

my $fh;
open($fh, "/dev/null") or open($fh, "nla0:") or (die "Can't open /dev/null.");`
....

Here we again face the question: What changes in older test files should be done in our Perl 7 research, and which should be done in Perl 5 blead?

I don't think the answer is clear-cut. For example, I myself saw merit in both sides of the argument in #188.

One could argue that getting rid of all bareword filehandles in the Perl core distribution test suite is a Good Thing because the test suite should display best practices.

However, I don't see any discussion of discouraging, deprecating, or fatalizing them in any of our Objectives on our roadmap. In other words, we've never put any of that "in scope" for "Perl 7."

So, so far my approach has been to address the warning that comes up when we turn on warnings-by-default in the simplest possible manner. It seems to me that quoting a bareword filehandle (e.g., try becomes 'try') is conceptually simpler than transforming the filehandle into a variable.

I reluctantly conclude that this should be raised on p5p list.

Thank you very much.
Jim Keenan

@jkeenan
Copy link
Collaborator Author

jkeenan commented Oct 5, 2020

no warnings and no strict affect variables. Could we not edit these variables directly?

@xsawyerx, Can you clarify what you mean by this? Perhaps by giving an example?

For instance, how would you address the warnings generated by running t/base/num.t in the alpha-dev-03-warnings branch?

@xsawyerx Still hoping for clarification on this.

Thanks.
jimk

@jkeenan
Copy link
Collaborator Author

jkeenan commented Oct 10, 2020

No recent comments. Proceeding to merge into alpha-dev-03-warnings. If you have any further issues, please open a new ticket specific to a particular test file.

Thank you very much.
Jim Keenan

@jkeenan jkeenan closed this Oct 10, 2020
@jkeenan jkeenan reopened this Oct 10, 2020
@jkeenan jkeenan merged commit 8f20f05 into alpha-dev-03-warnings Oct 10, 2020
@jkeenan jkeenan deleted the jkeenan/warnings-t-base branch October 10, 2020 17:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested or policy needs discussion warnings-by-default
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants