Skip to content

Add a PG Critic for checking that PG code conforms to best-practices in problem authoring. #1278

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

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from

Conversation

drgrice1
Copy link
Member

This is a complete rework of #1254, and is intended to replace that pull request.

This uses Perl::Critic and custom PG policies for Perl::Critic to analyze the code. The custom PG policies must be under the Perl::Critic::Policy to be loaded by Perl::Critic (they give no alternative for that). That means they are in the lib/Perl/Critic/Policy directory. Policies corresponding to everything that was attempted to be detected in #1254 have been implemented except for randomness and the "positive" features.

randomness of a problem is far more complicated than just checking if random, list_random, etc., are called.

The "positive" features were really going to be a problem if the intent of the score is to obtain a measure of how much work is needed to fix a problem and make it conform to current best-practices in problem authoring, and they were really just a "whose a good dog" sort of thing (so not needed). For example, a problem could do quite a bit wrong but have a custom checker. The custom checker score was really high, and so that would offset the things done wrong and it might end up with a score that is the same as for a problem that only does a few things wrong, but doesn't have a custom checker. So that first problem with the custom checker really needs a lot of work, but the second problem without the checker only needs trivial fixes. By only having a badness score you get a much clearer measure of how much is needed to update a problem to conform with current best-practices.

Basically, the way this works is that the code of a problem is first translated (via the default_preprocess_code method of the WeBWorK::PG::Translator package), then converted to a PPI::Document (the underlying library that Perl::Critic uses), and that is passed to Perl::Critic.

There are some utility methods provided in the WeBWorK::PG::Critic::Utils package that can be used by the PG policies. At this point those are getDeprecatedMacros, parsePGMLBlock, and parseTextBlock.

The getDeprecatedMacros method just lists the macros in the macros/deprecated directory.

The parsePGMLBlock method parses PGML contents, and actually uses PGML::Parse for the parsing, and returns PPI::Document representations of the content. At this point only command blocks are returned (perl content of [@ ... @] blocks), but more can be added as needed by the policies that are created.

The parseTextBlock method is similar but parses BEGIN_TEXT/END_TEXT blocks (and the ilk) using a simplified ev_substring approach. At this point only the contents of \{ ... \} blocks are returned, and other elements can be added later if needed.

Unfortunately, the parsePGMLBlock and parseTextBlock methods do not give proper positioning within the code, so the line and column numbers of the things in the return value will not be reliable. The only policy that uses these at this point is the Perl::Critic::Policy::PG::RequireImageAltAttribute policy and that just reports the violations as being inside the PGML or text block the violations are found in.

Also, the original untranslated code is passed to the policies and can be used if needed. The Perl::Critic::Policy::PG::ProhibitEnddocumentMatter is the only policy that uses this at this point.

Note that since this is just Perl::Critic this also reports violations of the core Perl::Critic policies (at severity level 4). However, there are policies that clearly don't apply to PG problem code, and so those are disabled. For instance, obviously use strict and use warnings can't be called in a problem, so the Perl::Critic::Policy::TestingAndDebugging::RequireUseStrict and Perl::Critic::Policy::TestingAndDebugging::RequireUseWarnings policies are disabled. The disabled policies start at line 57 of the WeBWorK::PG::Critic package. This may need tweaking as there may be other policies that need to be disabled as well, but those are the common violations that I have seen over the years using this for problems that should not apply to problems (I have used a form of this PG critic without the custom PG policies for some time now -- see https://github.com/drgrice1/pg-language-server).

Also note that since this is just Perl::Critic, you can also use ## no critic annotations in the code to disable policy violations for a specific line, the entire file, a specific policy on a specific line, etc. See https://metacpan.org/pod/Perl::Critic#BENDING-THE-RULES. For example, if you have a problem that is in the works and are not ready to add metadata, then add ## no critic (PG::RequireMetadata) to the beginning of the file, and you won't see the violations for having missing metadata. Note that the bin/pg-critic.pl script has a -s or --strict option that ignores all ## no critic annotations, and forces all policies to be enforced.

The result is a reliable, versatile, and extendable approach for critiquing problem code.

Since there was a desire to have a "problem score", that has been implemented. The score is implemented by setting the "explanation" of each violation as a hash which will have the keys score and explanation. The higher a violation score is the worse the violation is. Summing the scores for all violations gives the "problem score". The higher the score, the more work is needed to fix the problem. A score of 0 means the problem has no issues.

The explanation is of course a string that would be the usual explanation. This is a bit of a hack since Perl::Critic expects the violation to be either a string or a reference to an array of numbers (page numbers in the PBP book), but the explanation method of the Perl::Critic::Violation object returns the hash as is so this works to get the score from the policy.

Note that a policy can also add a sampleProblems key to this explanation hash. The value should be a reference to an array of references to two element arrays containing a sample problem title and sample problem path relative to the tutorial/sample-problems directory without the .pg extension. Of course these should be sample problems that demonstrate a way to fix the violation. These sample problems are linked to in the corresponding webwork2 pull request that implements the usage of this "PG Critic" in the problem editor.

drgrice1 added 5 commits July 16, 2025 09:31
This uses `Perl::Critic` and custom PG policies for `Perl::Critic` to
analyze the code.  The custom PG policies must be under the
`Perl::Critic::Policy` to be loaded by `Perl::Critic` (they give no
alternative for that).  That means they are in the
`lib/Perl/Critic/Policy` directory. Policies corresponding to everything
that was attempted to be detected in openwebwork#1254 have been implemented except
for `randomness`. `randomness` of a problem is far more complicated than
just checking if `random`, `list_random`, etc. are called.

Basically, the code of a problem is first translated (via the
`default_preprocess_code` method of the `WeBWorK::PG::Translator`
package), then converted to a `PPI::Document` (the underlying library
that `Perl::Critic` uses), and that is passed to `Perl::Critic`.

There are some utility methods provided in the
`WeBWorK::PG::Critic::Utils` package that can be used by the PG
policies. At this point those are `getDeprecatedMacros`,
`parsePGMLBlock`, and `parseTextBlock`.

The `getDeprecatedMacros` method just lists the macros in the
`macros/deprecated` directory.

The `parsePGMLBlock` method parses PGML contents, and actually uses
PGML::Parse for the parsing, and returns `PPI::Document` representations
of the content. At this point only command blocks are returned (perl
content of `[@ ... @]` blocks), but more can be added as needed by the
policies that are created.

The `parseTextBlock` method is similar but parses
`BEGIN_TEXT`/`END_TEXT` blocks (and the ilk) using a simplified
`ev_substring` approach.  At this point only the contents of `\{ ... \}`
blocks are returned, and other elements can be added later if needed.

Unfortunately, the `parsePGMLBlock` and `parseTextBlock` methods do not
give proper positioning within the code, so the line and column numbers
of the things in the return value will not be reliable.  The only policy
that uses these at this point is the
`Perl::Critic::Policy::PG::RequireImageAltAttribute` policy and that
just reports the violations as being inside the PGML or text block the
violations are found in.

Also, the original untranslated code is passed to the policies and can
be used if needed.  The `Perl::Critic::Policy::PG::ProhibitEnddocumentMatter`
is the only policy that uses this at this point.

Note that since this is just `Perl::Critic` this also reports violations
of the core `Perl::Critic` policies (at severity level 4).  However,
there are policies that clearly don't apply to PG problem code, and so
those are disabled.  For instance, obviously `use strict` and `use
warnings` can't be called in a problem, so the
`Perl::Critic::Policy::TestingAndDebugging::RequireUseStrict` and
`Perl::Critic::Policy::TestingAndDebugging::RequireUseWarnings` policies
are disabled. The disabled policies start at line 57 of the
`WeBWorK::PG::Critic` package.  This may need tweaking as there may be
other policies that need to be disabled as well, but those are the
common violations that I have seen over the years using this for
problems that should not apply to problems (I have used a form of this
PG critic without the custom PG policies for some time now -- see
https://github.com/drgrice1/pg-language-server).

Also note that since this is just `Perl::Critic`, you can also use
`## no critic` annotations in the code to disable policy violations for
a specific line, the entire file, a specific policy on a specific line,
etc.  See https://metacpan.org/pod/Perl::Critic#BENDING-THE-RULES. For
example, if you have a problem that is in the works and are not ready to
add metadata, then add `## no critic (PG::RequireMetadata)` to the
beginning of the file, and you won't see the violations for having
missing metadata.  Note that the `bin/pg-critic.pl` script has a `-s`
or `--strict` option that ignores all `## no critic` annotations, and
forces all policies to be enforced.

The result is a reliable, versatile, and extendable approach for
critiquing problem code.

Since there was a desire to have a "problem score" and to reward good
behavior that has been implemented.  That means that not all
"violations" are bad.  Some of them are good.  The score is implemented
by setting the "explanation" of each violation as a hash which will have
the keys `score` and `explanation`.  The score will be positive if the
"violation" is good, and negative otherwise.  The `explanation` is of
course a string that would be the usual explanation.  This is a bit of a
hack since `Perl::Critic` expects the violation to be either a string or
a reference to an array of numbers (page numbers in the PBP book), but
the `explanation` method of the `Perl::Critic::Violation` object returns
the hash as is so this works to get the score from the policy.

Although, I am wondering if this "problem score" is really a good idea.
If we do start using this and make these scores public, will a low score
on a problem deter usage of the problem? It seems like this might
happen, and there are basic but quite good problems that are going to
get low scores simply because they don't need complicated macros and
code for there implementation. Will a high score really mean that a
problem is good anyway? What do we really want these scores for?  Some
sort of validation when our problems get high scores because they
utilize the things that happen to be encouraged at the time?  I am
thinking that this "problem score" idea really was NOT a good idea, and
should be removed.

If the score is removed, then there is also no point in the "positive
violations".  Those simply become a "pat on the back" for doing
something right which is really not needed (in fact that is all they
really are even with the score in my opinion).

So my proposal is to actually make this a proper critic that just shows
the things in a problem that need improvement, and remove the score and
the "positive violations". That is in my opinion what is really
important here.
Now all violations are really violations.  There is no need for the
"positive violations".  The score is now purely a badness score.  The
higher the score, the more work is needed to fix the problem.  A score
of 0 means the problem has no issues.

The "positive violations" were really going to be a problem if the
intent of the score is to obtain a measure of how much work is needed to
fix a problem and make it conform to current best-practices in problem
authoring.  For example, a problem could do quite a bit wrong but have a
custom checker.  The custom checker score was really high, and so that
would offset the things done wrong and it might end up with a score that
is the same as for a problem that only does a few things wrong, but
doesn't have a custom checker.  So that first problem with the custom
checker really needs a lot of work, but the second problem without the
checker only needs trivial fixes.  By only having a badness score you
get a much clearer measure of how much is needed to update a problem to
conform with current best-practices.
…cript.

If this option is set, only PG critic policy violations (those in the
`Perl::Critic::Policy::PG` namespace) are shown and scored, and general
Perl critic policies are ignored.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants