-
-
Notifications
You must be signed in to change notification settings - Fork 77
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
drgrice1
wants to merge
6
commits into
openwebwork:develop
Choose a base branch
from
drgrice1:pg-critic
base: develop
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+1,078
−0
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
349cd53
to
a386d0e
Compare
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.
This is much more minimal.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a complete rework of #1254, and is intended to replace that pull request.
This uses
Perl::Critic
and custom PG policies forPerl::Critic
to analyze the code. The custom PG policies must be under thePerl::Critic::Policy
to be loaded byPerl::Critic
(they give no alternative for that). That means they are in thelib/Perl/Critic/Policy
directory. Policies corresponding to everything that was attempted to be detected in #1254 have been implemented except forrandomness
and the "positive" features.randomness
of a problem is far more complicated than just checking ifrandom
,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 theWeBWorK::PG::Translator
package), then converted to aPPI::Document
(the underlying library thatPerl::Critic
uses), and that is passed toPerl::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 aregetDeprecatedMacros
,parsePGMLBlock
, andparseTextBlock
.The
getDeprecatedMacros
method just lists the macros in themacros/deprecated
directory.The
parsePGMLBlock
method parses PGML contents, and actually uses PGML::Parse for the parsing, and returnsPPI::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 parsesBEGIN_TEXT
/END_TEXT
blocks (and the ilk) using a simplifiedev_substring
approach. At this point only the contents of\{ ... \}
blocks are returned, and other elements can be added later if needed.Unfortunately, the
parsePGMLBlock
andparseTextBlock
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 thePerl::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 corePerl::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, obviouslyuse strict
anduse warnings
can't be called in a problem, so thePerl::Critic::Policy::TestingAndDebugging::RequireUseStrict
andPerl::Critic::Policy::TestingAndDebugging::RequireUseWarnings
policies are disabled. The disabled policies start at line 57 of theWeBWorK::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 thebin/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
andexplanation
. 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 sincePerl::Critic
expects the violation to be either a string or a reference to an array of numbers (page numbers in the PBP book), but theexplanation
method of thePerl::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 thetutorial/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.