Perl::Critic policy

[xsawyerx]

I had worked in the past on a Perl::Lint policy. It's available here.

I want to use this ticket to collect what this should include (possibly multiple policies) so it could be coordinated, documented, and written:

• Detect and forbid param and params. (params('query') can be identified to recommend query_parameters instead.)
• Detect and forbid dance. (Suggest to_app instead.)
• ProhibitObsoleteImport: detects old import options.
• ProhibitObsoleteKeywords: detects keywords that are no longer supported.
• SuperfluousReturn: return send_error(...); or return redirect(...);.

[shumphrey]

How about:

return send_error(); # superfluous return

[xsawyerx]

Added!

[GeekRuthie]

I have started this module, with two policies, ProhibitDeprecatedKeywords, and ProhibitUnrecommendedKeywords. It's available via CPAN now: Perl::Critic::Dancer2. I'll be adding more later, and your recommendations/suggestions/pull requests are absolutely welcome!

[GeekRuthie]

I've added a policy, "ReturnNotNeeded", which handles superfluous returns.

I also have additional ideas:

• suggest send_as JSON instead of return to_json and similar constructs
• Suggest Test::WWW::Mechanize::PSGI instead of Test::Dancer2 or Plack::Test
• Fuss about long routes (configurable, default 100 lines). Rationale: If you separate route logic from the actual Doing Things logic, it makes unit testing both easier--to test the Dance, mock the Doing Things, and testing the Doing of Things becomes a straightforward unit test. Modularizing the application logic away from the routing lets you do this sanely.

[xsawyerx]

@GeekRuthie This is a great service to all Dancer2 users. Great job!

I have a few notes which might (or might not) help:

• For the deprecated and unrecommended keywords, I suggest indicating in the POD which keywords are checked and what they are replaced with.
• For params, I suggest adding a link to the appropriate document or this article on it. Changing it might require changing an assumption in the app itself. For example, an app that expected the same input parameter from either the query or the body should check both parameter sources or change the expectations (i.e., only accept it from one parameter source).
• Have you considered the following issues as possible policies?
  • ProhibitObsoleteImport: detect old. ignored imports like :syntax, :script, and :tests?
  • ProhibitBareImport: Importing with :nopragmas is often in bad form because you're making sure strict and warnings are not imported. If you were to write it, I'd recommend giving an example of how to do it without the import flag: package App; { use Dancer2; } ... - this will restrict strict and warnings to a small scope that ends immediately after, so it doesn't affect the rest of the code.

Regarding your ideas:

• I think the to_json -> send_as idea is interesting. Either I'm not big on it or I think it's brilliant, and I'm not sure which. :)
• Dancer2::Test is deprecated. I think both Plack::Test and Test::WWW::Mechanize::PSGI are good. I would recommend either.
• That sounds super awesome. There is a general Critic policy on McCabe complexity. Maybe its code could be used for the policy checking the complexity of each route.

Anyway, really cool work! :)