Bug #79504 for strictures: "smells like VCS"? really? this is not right; it is not even wrong

On Sep 7, 2012, at 18:09, "Chip Salzenberg via RT" <bug-strictures@rt.cpan.org> wrote: Show quoted text
This is the best possible heuristic (in the Klortho sense, indeed) that we've been able to discern for the purpose. We'd welcome a better one that doesn't sacrifice too much simplicity. This is very much a least worst sort of thing, and this is the least worst solution we have so far. Everything else fails in more common cases.

I'm trying to remain calm but this is just stupid. You can't argue the quality of the heuristic when the point of it is so deeply misguided. Read slowly: A module. Should not behave differently. Depending on whether $PWD has .git in it. This is very simple, it is obviously correct, and I can't imagine why you would imagine otherwise.

On Fri, Sep 07, 2012 at 06:17:44PM -0400, Chris Nehren via RT wrote: Show quoted text
Then perhaps the whole idea is flaed? I am with Chip on this one - it is incredibly icky. Combine this with the amount of fallout we have seen over the past year as more and more things start including Moo - it's a slippery slope. So how about making an explicit switch? A $HOME/.perl_strictures or somesuch. So you don't have to dick around with any envvars, and your dev-environment is your dev-environment. Thoughts?

On Fri, Sep 07, 2012 at 06:23:42PM -0400, Chip Salzenberg via RT wrote: Show quoted text
Please let's try to avoid rudeness. I don't think reading speed is a factor here. Show quoted text
It obviously isn't so obvious, or this discussion would not have arisen. The documentation clearly states that the intention is that behaviour is different when the author is running his own code (via a unit test, or in an "uninstalled" location. You may disagree as to the wisdom of this, but that's one of the express missions of this module.

On 9/7/2012 3:36 PM, Chris Nehren via RT wrote: Show quoted text
You know better than to imagine that a bug report is the Word Of Truth.

On Sat Sep 08 00:52:55 2012, CHIPS wrote: Show quoted text
For what it's worth, there actually is $ENV{PERL_STRICTURES_EXTRA} for that purpose.

On Fri, Sep 07, 2012 at 06:52:56PM -0400, Chip Salzenberg via RT wrote: Show quoted text
Yes, it sounds mildly counterintuitive, but this afaiu is *precisely* what the author wants as behavior. Hence leaving the option to explicitly turn this on for good is... good. Show quoted text
Why... ?

On Fri Sep 07 23:15:59 2012, RIBASUSHI wrote: Show quoted text
what Show quoted text
Just because he wants it doesn't mean he's right. Surely this has been obvious ever since the first badly written program. To make clear just how stupid this is: I will fork Moo before I let it stand. I'm a busy guy, but I *will* make the time. This is that stupid. Deeply and profoundly. Show quoted text
going Show quoted text
Because what I am developing and what I am using are two independent things. Sometimes more than two. So using one global flag in my $HOME to indicate which non-global mode I am in is stupid.

Here's my current train of thought - The extra testing is all stuff that only ever blows up at compile time; this is intentional. So your assertion that it's different code being tested is only sort of the case - none of the modules involved affect the final optree to my knowledge, so the author gets some additional compile time crashes which he/she then fixes, and the rest of the testing is completely valid for all environments. The point of the extra testing - especially 'no indirect' - is to catch mistakes that newbie users won't even realise are mistakes without help. For example, foo { ... }; where foo is an & prototyped sub that you forgot to import - this is pernicious to track down since all -seems- fine until it gets called and you get a crash. Worse still, you can fail to have imported it due to a circular require, at which point you have a load order dependent bug which I've seen before now -only- show up in production due to tiny differences between the production and the development environment. As such, in my experience so far the strictures extra testing has -avoided- production versus development differences, not caused them. Additionally, strictures' policy is very much "try and provide as much protection as possible for newbies - who won't think about whether there's an option to turn on or not" - so having only the environment variable is not sufficient to achieve that (I get to explain that you need to add 'use strict' at least once a week on freenode #perl - newbies sometimes completely skip steps because they don't understand that that step is important). I'd be open to having a '.no_strictures_extra' file that you create to disable the behaviour for repositories you want to deploy from. I'd be open to having 'use Moo -no_extra_tests;' or something of that ilk. I'm fairly open to pretty much any idea here, but the thing is, you were quite able to find the environment variable and to figure out what was going on. I want to make it easy for experts to figure out what's happening and make it do what they want. I really, *really* don't want to accept a change that means that people who haven't thought about what they want will stop getting the extra protections - and that's what the heuristic is for. I'm not claiming it's a good approach, but I -am- claiming it's definitely saved users from bugs that they would've found much harder to track down otherwise, and I'm unwilling to simply abandon that affordance. All thoughts gratefully received.

On Sat Sep 08 11:37:24 2012, MSTROUT wrote: Show quoted text
this) I'm sorry, Matt, I like you a lot, but you're pushing people to use Moo, Moo uses strictures, and strictures is being evil. Unless extra_testing is the default, there will be a fork. Unless the filesystem is no longer consulted, there will be a fork. This is non-negotiable.

On Sat Sep 08 15:25:40 2012, CHIPS wrote: Show quoted text
package Moo::NXT; sub import { $ENV{STRICTURES_EXTRA_TESTS} = 0; require Moo; goto &Moo::import; } should work fine to force the default the way you want for your code. I'll happily take a patch to avoid checking the filesystem if the env var's already been set explicitly. As for the rest - currently it seems to me that you're using "evil" to mean "annoys me due to behaviour designed to help newbies". You're an ex-pumpking. I'm allowed to expect you to read the docs. I -know- they won't. I, currently, believe that the positive value that newbies derive from this behaviour exceeds the negative value of the annoyance that it's going to cause people - and I've done my best to express how I came to that conclusion. You clearly believe that conclusion to be in error - could we please discuss *why* rather than playing chicken? I expressed my arguments at length with the intent that counterarguments to them could be provided. "this is not even wrong" and "this is evil" are somewhat useful to me to gauge the vehemence of your disagreement by, but fall a little short of actually being counterarguments that we can discuss :)

On Sat Sep 08 17:20:19 2012, CHIPS wrote: Show quoted text
They don't. They won't. strictures doesn't do that. The only thing that strictures does is: If .git is present in pwd, *and* the file in question is in t/, xt/, lib/ or blib/ - and note that the regexp is anchored at the front, so it has to have been loaded as a path relative to pwd - extra testing is enabled. /usr/bin/splain will not be affected. /usr/blah/blah/blah/lib/Some/Module.pm will not be affected. I'm not sure where you developed the idea that they would be, unless you're actually filing a bug against a pre-1.004 version without testing the current logic. Now, I *can* see an argument that for people deploying straight off a checkout, scripts shouldn't be subject to the extra testing, so I'd suggest therefore augmenting the logic to check if $0 starts with 'lib/' or 't/' - this should ensure we activate both on running of tests and on casual compilation checks of a single module in the application's tree - i.e. our $Smells_Like_VCS = ($0 =~ /^(t|lib)\// and ...current checks...); and I think that would be a good thing; it'd also avoid useless stat()s in cases where we probably weren't going to turn anything on. However, you appear to be utterly confused about how strictures currently works, so I'd ask that you stop, step back, take a deep breath, and give me -actual- examples of the behaviour that you can -demonstrate- against the latest release. We can't usefully argue about whether strictures is right if you're accusing it of being wrong for things that it doesn't actually do. Also, I can't turn things that don't actually happen into test cases if you manage to produce a genuine problem.

On Sun Sep 09 09:27:48 2012, MSTROUT wrote: Show quoted text
Just spotted lib::if::dev on CPAN, which checks for the existence for Makefile.PL, Build.PL, or dist.ini. That sounds like a good probe to distinguish between deploy-from-git and develop-in-git. Leon

On Mon Sep 10 05:55:56 2012, LEONT wrote: Show quoted text
Except that people often retain one of the first two for 'make installdeps' or './Build --installdeps' or 'cpanm --installdeps .' - and equally people often delete them because they don't understand and then deploy via FTP :) Basically, the key problem here is that some people are confusing their version control system and a deployment tool, and we need to accommodate that and cause them a minimum of pain until they learn better.