Re: Perl::Critic Tests

On Thu, Apr 27, 2017 at 1:39 PM, R. Ransbottom <..hidden..> wrote:

On Tue, Apr 25, 2017 at 05:47:42PM +0100, Nick Prater wrote:
> On 2017-04-25 04:12, R. Ransbottom wrote:
> > Don't do this, please.
> >
> > That is Perl novice thinking which shows a poor understanding of Perl's

> I'm afraid I don't follow. Don't do what? The contributors to this
> thread are not Perl novices. It may not be your intention, but your
> comment reads to me as disparaging and belittling to those that have
> ...

My apologies for brutishness and obscureness. I appreciate the kindness
of your rebuke. I was disparaging the thinking; if I thought poorly
of skills of the people involved I would not be advocating for standard
practices that embrace more nuanced code rather I would push for practices
that disallow them for safety's sake.

Please don't do this: not "enforce the ProhibitExplicitReturnUndef policy
and add a comment to the tests to explain why."

> I consider that we should strive for code that is as obvious and
> accessible as possible, even to a novice. That encourages contributions,
> is less prone to errors and more maintainable. The very point of this
> discussion is to promote good standards of coding.

I don't agree entirely. I don't think a complex sql statement should
be simplified for me as a beginner. I would prefer that good journeyman
code be written--that good code is modelled for me if I care to learn.

Right. We agree here, but no code should be more complex than the absolute minimum required. By which I mean that there's no value in creating more complex code than the problem needs.

To a beginner, the obviously complex code is less dangerous than the code
that seems straightforward but is not.

So true. Our point here is that even a not-so-beginner could very well be deceived by the multi-context effects of a function used in a context that doesn't look like a list context (as the context that was quoted from Matt). Emperical evidence has it that even not-so-beginners are confused about this type of code. That's exactly the reason we do *not* want to allow the use of this specific coding style: the code seems straightforward but is not.

I think the simple "return undef" models a bad practice.

Ok. I personally think that the 'return undef' models are good practice, because they provide explicit means to identify that the routine was meant to be a scalar-returning function.

Moreover, Matt was quoting from empirical evidence and personal experience where a security audit would have rejected his code if he had not had an explicit 'return undef' line in his code (because otherwise it *would* have been liable to security problems.)

It is certainly
reasonable to write that in the situation when undefs have to
be treated as data values headed to or from SQL.

The choices seem to be:

return undef; # no change; treat as acceptable
undef; ## no critic
return undef; ## no critic

return scalar undef;
die "bad context blah blah" if wantarray; return;
caller_deals_with_normal_return_magic( returner());

If making a change, I would suggest the 'return scalar undef' which puts the
solution in code. I expresses the intent. I don't think any of the first
four choices expresses the intent clearly for novices.

Could you go into more detail what you mean here? What do you think could be the intent of the code? More specifically, I don't understand why (5) is different than (1), other than that (5) will throw an error on the following:

call_a_function({ key_1 => call_dying_in_list_context_function() });

whereas (1) will simply call the outer function with the hashref "{ key_1 => undef }"

The last two are logic changes so I rule them out for refactoring
working code.

> > Matt forgot to 'use warnings;'. Ouch! Had me going for a minute.
> > That example has nothing to do with the bare return statement.

> To clarify, Matt didn't forget. He posted a contrived example in an

That was supposed to be a device, but being too-literal myself I should
have crafted it better. I did not think Matt forgot; but for me warnings
have trivialized the problem to extent that I haven't thought about it.

> ...

> The code will still execute even with 'use warnings', so that doesn't
> protect us against such errors.

I find 'use warnings' helpful. I find a more draconian or nagging
mechanism helpful too.

Yes. We require 'use warnings;' as well as 'use strict;' on all our code (except on the old code where the code will simply stop working if we do -- which is why we still want to replace it all).

So, we use the measures available to us. There are however many more "draconian and nagging" methods, which include code policies which are part of the continuous integration validations: code which doesn't adhere to the policy won't be allowed into the code base and compliance will be checked before the code enters.

> >> I prefer an explicit "return undef;" as I think it's clearer, more
> >> obvious about the intention and less magical. Eric suggests we
> >> problably
> >> want to switch from "no return at all" to "return undef".

> > The problem with this _expression_ of intent is that it is buried at
> > the bottom of a function that someone wants to use not to read.

> Is there a better way to express the intent?

caller_deals_with_normal_return_magic( returner());

would be the best.

That all such callee's be private subs would be an amelioration.

But isn't this putting a burden on the caller (and its programmer) who should know about the exact implications of context in Perl, while simply returning either an array/list or a scalar makes debugging easy and doesn't require this level of understanding of Perl -- thus allowing a much lower barrier of entry to our codebase or new contributors.

> As a counter-example, what about when building an array of query
> parameters that use undef to represent a NULL value? In these situations
> you do not what those elements to be magically collapsed.

I looked at about 15 return undefs in the code but didn't find code
that looked like it was used toward this purpose.

But isn't that the wrong way around? Should you not have checked the 'return;'s to see if they are being used in a mixture of list and scalar context? It's the 'return;' type of syntax which is error prone, not the 'return undef;' syntax.

> The reason for documenting rather than just dropping it is so that
> others, in future, can benefit from our discussion and reasoning,

And hopefully our subsequent actions.

Thanks for your explanation, hoping for your followup,

Bye,

Erik.

http://efficito.com -- Hosted accounting and ERP.

Robust and Flexible. No vendor lock-in.