Bug #69546 for Perl-Critic: Remove Perl::Critic::Policy::Miscellanea::RequireRcsKeywords

[...] Hmm, I just looked into PBP p.441, and my copy just tells something about using revision control systems, but not anything about using RCS keywords. So maybe at least the reference to PBP should be removed here. Regards, Slaven

I would vote again for removing this policy and starting fresh. While the policy of requiring a version control system is a good one, the implementation doesn't work well. Furthermore, it mixes up the far more contentious issue of requiring commit IDs in every source file. * It's poorly named. The policy is about making sure the user is using a version control system. That it's checking for RCS keywords is just the mechanism, and not a very good one. The name should reflect the policy, not the mechanism. * It conflates two policies. 1. Use a version control system 2. Have a way to map the file back to a particular commit. These should be separate. 2 is difficult in many version control systems, if not impossible, and is far less important than having a version control system. * The mechanism of detection is invasive. There are far better ways of detecting if a project is under version control than requiring the author to pepper every document with keywords. If you want the author to use keywords, make it a separate policy. * It generates a very high rate of false negatives. If you're using anything but CVS or Subversion you're out of luck or have to turn on emulation just to satisfy the policy. It will only get worse. While perlcritic is primarily a file-based beast, and only deals with reading the code, for this one to work it must look outside the file. Either for... * a `.blah` directory in the cwd or above This covers SVN, git and most of the rest of the version control systems introduced in the last five/ten years. * a corresponding `,v` file RCS * run a command CVS and SVK As an alternative, rather than Perl::Critic trying to guess what VCS the project is using, the new RequireVCS policy would warn unless .perlcritic has been configured with what VCS is in use. Then it could choose an appropriate method of checking. It could also be configured with the path to the proper program to run, rather than grovelling up the directory tree looking for .blah directories. A resurrected RequireVcsKeywords would also key off this information (rather than having it told redundantly). It would then check for VCS keywords appropriately, which includes not requiring them at all if your VCS doesn't support them. RequireVCS could be given a higher priority and RequireVcsKeywords a lower one.

On Feb 22, 2012, at 1:03 PM, Michael G Schwern via RT wrote: Show quoted text
Well, I don't entirely agree with those statements. But I do agree that the Policy doesn't actually enforce the best-practice that Conway is espousing. And yes, I think we can do better here. Show quoted text
Looking for artifacts of a VCS system is good. But I think actually checking if the file is under version control is even better. Basically, do the moral equivalent of "svn stat $file" using the appropriate VCS tool. Either way, it implies that this Policy will only be applied to files that came directly from a VCS. This will throw a false violation if you run the Policy on the stuff in the blib/ directory (as Test::Perl::Critic does). I'm not sure how to get around that. Show quoted text
Yes, this seems like the right thing to do. But it leads me to feel that such a Policy does not belong in the core. Presently, all the core policies are zero-conf. They all do something useful right out of the box (even if you don't agree with the default config). I would like to keep it that way. Requiring configuration for some Policies is fine. But from a usability perspective, I feel it automatically relegates them to a non-core distribution, so novice users will typically not have access to this Policy. Unfortunate, but necessary I think. Show quoted text
In the current architecture, each Policy is isolated and independent. They don't talk to each other and they don't share configuration information. So tying these Policies together directly will either be a hack, or we'll have to re-architect quite a bit. Another approach would be to combine the Policies together. This obviously makes the Policy more complex, and again, it conflates multiple issues. But it might be a more natural solution, given the current architecture. There might be other approaches. I'll have to think about it a bit. -Jeff

Something I realized while writing up this reply: this is NOT because I think P::C needs a policy to detect a VCS. This is really about quieting the annoying RequireRcsKeywords policy by eliminating the excuse that it's a proxy for detecting a vcs. If RequireRcsKeywords was turned off by default (or just stopped bothering people not using CVS/SVN) I'd say P::C doesn't need RequireVcs. Nobody needs a policy to tell them a project is not using a vcs, it's painfully obvious. Nobody who is failing to use a vcs is likely to be using perlcritic nor at medium severity nor be swayed by a policy. Therefore, a simple solution presents itself. 1. Junk the idea of a policy to check if you're using a VCS. It doesn't detect anything which isn't already obvious. It doesn't help the people who need help. 2. Change RequireRcsKeywords to ONLY be about RCS keywords. 3. Move the VCS detection code into VCS::Detect. 4. Have RequireRcsKeywords use VCS::Detect to check for an applicable VCS. Non-applicable VCS get an automatic pass. 5. Reduce RequireRcsKeywords to the lowest severity. RCS keywords just aren't that important and the cost of satisfying the policy is high. (Not strictly necessary) 5b. If VCS::Detect detects no vcs, RequireRcsKeywords is satisfied. This lets RequireRcsKeywords work on distributed code. I like that idea. What do you think? Here's my detailed reply I wrote up before realizing the above. It's largely irrelevant if we take the above path. ---------------------------------------------------------------------- The implementation has changed since I wrote it up. It's now zero-conf (giving it an explicit type is optional) and now works just by looking for VCS files and directories. It only detects if the file is in a version controlled tree, not that the file is actually under version control. This is preferred behavior and I'll explain later. It works for RCS, CVS, SVN, Mercurial, Git, Bazaar and Perforce. Others can be added later. SVK cannot be detected in this manner so you get an automatic pass if you tell it you're using SVK (I doubt there's many users left). Now that the policy is zero-conf and eshews external programs, it works like any other policy except in one major respect: it doesn't look at the code but the surrounding environment. The largest consequence is the policy will not work on shipped code. Perl::Critic should be an author only test, but this may cause complications. If anything argues against inclusion in the core, it's this. Separating RequireVcs from RequireRcsKeywords has the benefit that the severity of RequireVcs can be raised to medium. Not having a version control system is fairly severe and it won't generate so many false negatives. It's also been moved to the "maintenance" theme. RequireRcsKeywords is now at lowest severity reflecting both its high cost making the policy happy, narrow scope of use (ie. only RCS, CVS and SVN users) and low impact. It's also been removed from the PBP category. This makes the "low" severity more useful out of the box by removing a common false negative. I'm considering moving the VCS detection code into its own module, VCS::Detect. RequireRcsKeywords could then make use of that to determine if an appropriate version control system is in use and give other version control systems a pass. Show quoted text
Yes. I have developed a common .perlcritic for my projects which bends it to my will, so I have a solution, but bad defaults effect everyone. Show quoted text
I really doubt this policy will have any real impact. A missing VCS is dead obvious, I don't need perlcritic to tell me there isn't one in use. Anyone who isn't using version control is A) unlikely to be using perlcritic B) unlikely to be using it at medium severity and C) swayed by a policy to start using version control. This is about eliminating the annoying RcsKeywords policy. If RequireRcsKeywords was turned off by default (or just stopped bothering people not using CVS/SVN) I'd say P::C doesn't need RequireVcs. Show quoted text
Yes. This latest one came up from a discussion on Stack Overflow about ProhibitReverseSortBlock. http://stackoverflow.com/questions/9398789/how-can-i-sort-one-key-ascending-another-descending-without-using-b-compariso It lead to a number of people commenting they don't use perlcritic, or avoid anything but the highest severity, because the default policies get a little rediculous: the policy itself is too picky (ex. RcsKeywords), or the implementation is insufficient to properly judge the code (ex. ProhibitReverseSortBlock), or the default config is unrealistic (ex. rejection of 36129 to fix ProhibitReverseSortBlock). So I decided to fix one. tl;dr discussion of why it's a Good Thing to not check each individual file follows... I rejected the command-based implemenation for several reasons. The first broad category is "technical problems with grovelling around for executables". * The policy has to guess that an external executable is what it thinks it is. This could lead to unwanted side effects. * The executable my be under a different name or outside the PATH. This would require the user to configure a path for each VCS type they might work with, annoying. Worse, the configuration is *per user* not per project. There is currently no way to have both per user and per project perlcritic configuration. * The executable may not exist at all. For example, if a Windows user is using TortoseSVN they don't have a command line executable. The more damning problem appears when you examine a perlcritic workflow. 1. Write a new file. 2. Run perlcritic on it. 3. Test it (which might run Test::Perl::Critic). 4. Add and commit it. If the policy is "this individual file is under version control" then the above workflow does not work. Each file must be added BEFORE critic is run otherwise you get false negatives. This is undesirable. Detecting an added but never committed file is a bit more complicated even if you have commands, and impossible otherwise. There's also reasons why you'd want to run perlcritic over files which will never be checked in. You mentioned blib/ and other generated files. One could put a "## no critic" in each generated file, but that's trouble particularly for blib. In the end, the policy is about making sure you're using a version control system, not telling you how to use it in detail. Version control systems are already straying outside Perl::Critic's scope and the policy's qualifications are best left broad. As every file is checked, individual false positives are not important.

Le 2012-02-26 01:10:32, jeff@imaginative-software.com a écrit : Show quoted text
I agree. And the solution that schwern is building (detecting a VCS) would be bad anyway. Because P::C policies should be about Perl code quality, not about development practices in general. P::C policies should stay VCS-agnostic and still work outside the VCS. For example, I think that a good practice is to make a distribution release ("./Build dist" / "make dist") only from a export of the repository ("svn export") rather than from a working directory ("svn checkout") that may not be clean. In this context there is no .svn directories to detect. But P::C policies should work either from the VCS or not. I already see too many Dist::Zilla-built distributions that enforce using a Git repository hosted on GitHub (I admit to use this on my own projects because it is convenient *for me as the original author*). And I don't want this (VCS dependency) in Perl::Critic. -- Olivier Mengué - http://perlresume.org/DOLMEN

Le 2012-02-27 22:07:34, jeff@imaginative-software.com a écrit : Show quoted text
I fear through your words that you may be tempted to not distribute in the CPAN distribution some code that is used to generate Perl::Critic (I'm not talking about dependencies available on the CPAN). I insist that all the "hand-written" code used to generate the live code of a CPAN distribution must be in the distribution itself. Beside my personal opinion on that matter, it is also the opinion of the Debian project. See the Debian Free Software Guidelines http://www.debian.org/social_contract.en.html So even if patching from the CPAN dist is not anymore the common practice, it should stay doable. Breaking that would break one of the unwritten rules of the CPAN, and would also break distributions of the project on free software platforms. -- Olivier Mengué - http://perlresume.org/DOLMEN

This seems to have drifted well off-topic into version control practices well out of the purview of Perl::Critic. In the interest of getting something concrete out of this, I'd like to try and wrap it up. On the topic of what to do about RequireRcsKeywords it seems there's broad consensus that we should remove it... except for Elliot who will have to carry on knowing people can use git and Perl::Critic won't spit at them for it. On the topic of RequireVcs it also seems there's broad consensus that Perl::Critic shouldn't be in the game of detecting version control nor the author's environment in general. Also the discussion shows there's many different ideas about good version control practice, and some are not very compatible with RequireVcs. So it should be canned. So... I will... * Delete RequireRcsKeywords * If the Perl::Critic::More folks want to adopt it, they can. * Drop RequireVcs * I'll move the guts into VCS::Detector And then we can close this ticket and stop expending energy on it.

RequireRcsKeywords has been removed from the core Perl-Critic distribution in version 1.118. It will now ship with Perl-Critic-More at some point in the future. Right now, that distribution needs some clean-up work before I can ship it. -- Jeffrey Thalhammer Imaginative Software Systems www.imaginative-software.com