Merge pull request #2768 from drgrice1/pg-critic

somiaj · web-flow · commit 2bd468a3e4cf · 2026-01-01T07:52:10.000-07:00
Add "PG Critic" to the PG problem editor.
diff --git a/bin/check_modules.pl b/bin/check_modules.pl
@@ -88,6 +88,7 @@ =head1 DESCRIPTION
 	Net::OAuth
 	Opcode
 	Pandoc
+	Perl::Critic
 	Perl::Tidy
 	PHP::Serialization
 	Pod::Simple::Search
diff --git a/htdocs/js/PGProblemEditor/pgproblemeditor.js b/htdocs/js/PGProblemEditor/pgproblemeditor.js
@@ -161,6 +161,15 @@
 			?.addEventListener('change', () => (deleteBackupCheck.checked = true));
 	}
 
+	const renderArea = document.getElementById('pgedit-render-area');
+
+	const scrollToRenderArea = () => {
+		// Scroll to the top of the render window if the current scroll position is below that.
+		const renderAreaRect = renderArea.getBoundingClientRect();
+		const topBarHeight = document.querySelector('.webwork-logo')?.getBoundingClientRect().height ?? 0;
+		if (renderAreaRect.top < topBarHeight) window.scrollBy(0, renderAreaRect.top - topBarHeight);
+	};
+
 	// Send a request to the server to perltidy the current PG code in the CodeMirror editor.
 	const tidyPGCode = () => {
 		const request_object = { courseID: document.getElementsByName('courseID')[0]?.value };
@@ -235,15 +244,43 @@
 			.catch((err) => showMessage(`Error: ${err?.message ?? err}`));
 	};
 
+	// Send a request to the server to run the PG critic in the CodeMirror editor.
+	const runPGCritic = () => {
+		const request_object = { courseID: document.getElementsByName('courseID')[0]?.value };
+
+		const user = document.getElementsByName('user')[0];
+		if (user) request_object.user = user.value;
+		const sessionKey = document.getElementsByName('key')[0];
+		if (sessionKey) request_object.key = sessionKey.value;
+
+		request_object.rpc_command = 'runPGCritic';
+		request_object.pgCode =
+			webworkConfig?.pgCodeMirror?.source ?? document.getElementById('problemContents')?.value ?? '';
+
+		fetch(webserviceURL, { method: 'post', mode: 'same-origin', body: new URLSearchParams(request_object) })
+			.then((response) => response.json())
+			.then((data) => {
+				if (data.error) throw new Error(data.error);
+				if (!data.result_data) throw new Error('An invalid response was received.');
+				renderArea.innerHTML = data.result_data.html;
+				scrollToRenderArea();
+			})
+			.catch((err) => showMessage(`Error: ${err?.message ?? err}`));
+	};
+
 	document.getElementById('take_action')?.addEventListener('click', async (e) => {
-		if (document.getElementById('current_action')?.value === 'format_code') {
+		if (document.getElementById('current_action')?.value === 'code_maintenance') {
 			e.preventDefault();
-			if (document.querySelector('input[name="action.format_code"]:checked').value == 'tidyPGCode') {
+			if (document.querySelector('input[name="action.code_maintenance"]:checked').value === 'tidyPGCode') {
 				tidyPGCode();
 			} else if (
-				document.querySelector('input[name="action.format_code"]:checked').value == 'convertCodeToPGML'
+				document.querySelector('input[name="action.code_maintenance"]:checked').value === 'convertCodeToPGML'
 			) {
 				convertCodeToPGML();
+			} else if (
+				document.querySelector('input[name="action.code_maintenance"]:checked').value === 'runPGCritic'
+			) {
+				runPGCritic();
 			}
 			return;
 		}
@@ -306,7 +343,6 @@
 	});
 
 	const renderURL = `${webworkConfig?.webwork_url ?? '/webwork2'}/render_rpc`;
-	const renderArea = document.getElementById('pgedit-render-area');
 	const fileType = document.getElementsByName('file_type')[0]?.value;
 
 	// This is either the div containing the CodeMirror editor or the problemContents textarea in the case that
@@ -391,11 +427,7 @@
 		}
 
 		adjustIFrameHeight();
-
-		// Scroll to the top of the render window if the current scroll position is below that.
-		const renderAreaRect = renderArea.getBoundingClientRect();
-		const topBarHeight = document.querySelector('.webwork-logo')?.getBoundingClientRect().height ?? 0;
-		if (renderAreaRect.top < topBarHeight) window.scrollBy(0, renderAreaRect.top - topBarHeight);
+		scrollToRenderArea();
 	});
 
 	const render = () =>
diff --git a/lib/WeBWorK/ContentGenerator/Instructor/PGProblemEditor.pm b/lib/WeBWorK/ContentGenerator/Instructor/PGProblemEditor.pm
@@ -90,7 +90,7 @@ the submit button pressed (the action).
     Requested actions and aliases
         View/Reload                action = view
         Generate Hardcopy:         action = hardcopy
-        Format Code:               action = format_code
+        Code Maintenance:          action = code_maintenance
         Save:                      action = save
         Save as:                   action = save_as
         Append:                    action = add_problem
@@ -118,15 +118,15 @@ use SampleProblemParser        qw(getSampleProblemCode generateMetadata);
 use constant DEFAULT_SEED => 123456;
 
 # Editor tabs
-use constant ACTION_FORMS => [qw(view hardcopy format_code save save_as add_problem revert)];
+use constant ACTION_FORMS => [qw(view hardcopy code_maintenance save save_as add_problem revert)];
 use constant ACTION_FORM_TITLES => {
-	view        => x('View/Reload'),
-	hardcopy    => x('Generate Hardcopy'),
-	format_code => x('Format Code'),
-	save        => x('Save'),
-	save_as     => x('Save As'),
-	add_problem => x('Append'),
-	revert      => x('Revert'),
+	view             => x('View/Reload'),
+	hardcopy         => x('Generate Hardcopy'),
+	code_maintenance => x('Code Maintenance'),
+	save             => x('Save'),
+	save_as          => x('Save As'),
+	add_problem      => x('Append'),
+	revert           => x('Revert'),
 };
 
 my $BLANKPROBLEM = 'newProblem.pg';
@@ -847,9 +847,9 @@ sub view_handler ($c) {
 	return;
 }
 
-# The format_code action is handled by javascript.  This is provided just in case
+# The code_maintenance action is handled by javascript.  This is provided just in case
 # something goes wrong and the handler is called.
-sub format_code_handler { }
+sub code_maintenance_handler { }
 
 sub hardcopy_handler ($c) {
 	# Redirect to problem editor page.
diff --git a/lib/WebworkWebservice.pm b/lib/WebworkWebservice.pm
@@ -255,6 +255,7 @@ sub command_permission {
 		putPastAnswer     => 'problem_grader',
 		tidyPGCode        => 'access_instructor_tools',
 		convertCodeToPGML => 'access_instructor_tools',
+		runPGCritic       => 'access_instructor_tools',
 
 		# WebworkWebservice::RenderProblem
 		renderProblem => 'webservice_render_problem',
diff --git a/lib/WebworkWebservice/ProblemActions.pm b/lib/WebworkWebservice/ProblemActions.pm
@@ -8,6 +8,7 @@ use Data::Structure::Util qw(unbless);
 
 use WeBWorK::PG::Tidy          qw(pgtidy);
 use WeBWorK::PG::ConvertToPGML qw(convertToPGML);
+use WeBWorK::PG::Critic        qw(critiquePGCode);
 
 sub getUserProblem {
 	my ($invocant, $self, $params) = @_;
@@ -165,4 +166,18 @@ sub convertCodeToPGML {
 
 }
 
+sub runPGCritic {
+	my ($invocant, $self, $params) = @_;
+
+	return {
+		ra_out => {
+			html => $self->c->render_to_string(
+				template   => 'ContentGenerator/Instructor/PGProblemEditor/pg_critic',
+				violations => [ critiquePGCode($params->{pgCode}) ]
+			)
+		},
+		text => 'The script pg-critic has been run successfully.'
+	};
+}
+
 1;
diff --git a/templates/ContentGenerator/Instructor/PGProblemEditor/code_maintenance_form.html.ep b/templates/ContentGenerator/Instructor/PGProblemEditor/code_maintenance_form.html.ep
@@ -0,0 +1,47 @@
+% last unless $c->{is_pg};
+<div>
+	<div class="form-check">
+		<%= radio_button 'action.code_maintenance' => 'tidyPGCode',
+			id => 'action_code_maintenance_perltidy', class => 'form-check-input', checked => undef  =%>
+		<%= label_for 'action_code_maintenance_perltidy', class => 'form-check-label', begin =%>
+			<%== maketext('Reformat the code using perltidy.') =%>
+		<% end =%>
+		<a class="help-popup" data-bs-content="<%== maketext('Perltidy is a reformatting '
+			. 'function that attempts to format code in a standard way.  It does not change '
+			. 'the functionality of the code and in general is desired to have a common problem layout.') =%>"
+			data-bs-placement="top" data-bs-toggle="popover" role="button">
+			<i aria-hidden="true" class="fas fa-question-circle"></i>
+			<span class="visually-hidden"><%= maketext('Perltidy Help') %></span>
+		</a>
+	</div>
+	<div class="form-check">
+		<%= radio_button 'action.code_maintenance' => 'convertCodeToPGML',
+			id => 'action_code_maintenance_convert_PGML', class => 'form-check-input'=%>
+		<%= label_for 'action_code_maintenance_convert_PGML', class => 'form-check-label', begin =%>
+			<%== maketext('Convert the code to PGML') =%>
+		<% end =%>
+		<a class="help-popup" data-bs-content="<%== maketext('This option converts the text blocks '
+			. 'in the problem code to PGML and updates the loadMacros to include PGML and drop others. '
+			. 'This can be used as a first pass of the conversion, however the author will still need '
+			. 'to ensure the problem functions.  One area of attention should be the answer blanks, '
+			. 'which may not be converted correctly.') =%>"
+			data-bs-placement="top" data-bs-toggle="popover" role="button">
+			<i aria-hidden="true" class="fas fa-question-circle"></i>
+			<span class="visually-hidden"><%= maketext('PGML Conversion Help') %></span>
+		</a>
+	</div>
+	<div class="form-check">
+		<%= radio_button 'action.code_maintenance' => 'runPGCritic',
+			id => 'action_code_maintenance_run_pgcritic', class => 'form-check-input'=%>
+		<%= label_for 'action_code_maintenance_run_pgcritic', class => 'form-check-label', begin =%>
+			<%== maketext('Analyze code with PG Critic') =%>
+		<% end =%>
+		<a class="help-popup" data-bs-content="<%== maketext('This option analyzes the code with PG Critic '
+			. 'which gives suggestions on using modern PG constructs and ensures that the code conforms '
+			. 'to current best-practices.') =%>"
+			data-bs-placement="top" data-bs-toggle="popover" role="button">
+			<i aria-hidden="true" class="fas fa-question-circle"></i>
+			<span class="visually-hidden"><%= maketext('PG Critic Help') %></span>
+		</a>
+	</div>
+</div>
diff --git a/templates/ContentGenerator/Instructor/PGProblemEditor/format_code_form.html.ep b/templates/ContentGenerator/Instructor/PGProblemEditor/format_code_form.html.ep
diff --git a/templates/ContentGenerator/Instructor/PGProblemEditor/pg_critic.html.ep b/templates/ContentGenerator/Instructor/PGProblemEditor/pg_critic.html.ep
@@ -0,0 +1,56 @@
+% Perl::Critic::Violation::set_format('%m at line %l, column %c.');
+%
+<div class="m-3 overflow-auto">
+	<h2><%= maketext('PG Critic Violations') %></h2>
+	% my @pgCriticViolations = grep { $_->policy =~ /^Perl::Critic::Policy::PG::/ } @$violations;
+	% if (@pgCriticViolations) {
+		<h3 class="mt-2"><%= maketext('The following PG issues should be fixed:') %></h3>
+		<ul class="list-group">
+			% for (@pgCriticViolations) {
+				<li class="list-group-item">
+					<div><%= $_->to_string %></div>
+					<div>
+						<%= $_->explanation->{explanation} %>
+						<%== maketext(
+							'See [_1].',
+							link_to(
+								($_->policy =~ s/^Perl::Critic::Policy:://r)
+									=> pod_viewer => { filePath => 'lib/' . ($_->policy =~ s/::/\//gr) . '.pm' },
+								target => '_blank'
+							)
+						) =%>
+					</div>
+					% if (ref($_->explanation->{sampleProblems}) eq 'ARRAY' && @{ $_->explanation->{sampleProblems} }) {
+						<div>
+							<%== maketext('Related sample [plural,_1,problem]:',
+								scalar(@{ $_->explanation->{sampleProblems} })) %>
+							<%= c(map {
+								link_to(
+									$_->[0] => sample_problem_viewer => { filePath => $_->[1] },
+									target => '_blank'
+								)
+							} @{ $_->explanation->{sampleProblems} })->join(', ') =%>
+						</div>
+					% }
+				</li>
+			% }
+		</ul>
+	%}
+	% my @perlCriticViolations = grep { $_->policy !~ /^Perl::Critic::Policy::PG::/ } @$violations;
+	% if (@perlCriticViolations) {
+		<h3 class="mt-2"><%= maketext('The following general Perl issues should be fixed:') %></h3>
+		<ul class="list-group">
+			% for (@perlCriticViolations) {
+				<li class="list-group-item">
+					<div><%= $_->to_string %></div>
+					<div>
+						See <%= link_to(
+							($_->policy =~ s/^Perl::Critic::Policy:://r) => 'https://metacpan.org/pod/' . $_->policy,
+							target => '_blank'
+						) %>.
+					</div>
+				</li>
+			% }
+		</ul>
+	%}
+</div>
diff --git a/templates/HelpFiles/InstructorPGProblemEditor.html.ep b/templates/HelpFiles/InstructorPGProblemEditor.html.ep
@@ -156,19 +156,22 @@
 			. 'generating the PDF file using LaTeX.') =%>
 		</p>
 		<p>
-		<%= maketext('You can also click "Edit Selected Theme" to edit a hardcopy theme.  The new theme will be saved to '
-			. 'the templates/hardcopyThemes folder.') =%>
+		<%= maketext('You can also click "Edit Selected Theme" to edit a hardcopy theme.  The new theme will be saved '
+			. 'to the templates/hardcopyThemes folder.') =%>
 		</p>
 	</dd>
 
-	<dt><%= maketext('Format Code') %></dt>
+	<dt><%= maketext('Code Maintenance') %></dt>
 	<dd>
-		<%= maketext('Reformat the code using perltidy or a conversion to PGML. Using perltidy will change the code '
-			. 'in the editor window, and save changes to the temporary file. In some cases (if the code contains '
-			. 'backslashes or double tildes) this can result in odd spacing in the code. The convert to PGML '
-			. 'feature changes the code in text blocks in the code to use PGML features. Generally the conversion of '
-			. 'many of the formatting and LaTeX is performed correctly, however answer blanks need attention. In '
-			. 'either case, make sure to inspect the formatted code, and edit further or revert if needed.') =%>
+		<%= maketext('Three code maintenance tools can be utilized. First, the code can be reformatted using perltidy. '
+		. 'Using perltidy will change the code in the editor window, and save changes to the temporary file. In some '
+		. 'cases (if the code contains backslashes or double tildes) this can result in odd spacing in the code. '
+		. 'Second the code can be converted to PGML. This changes the code in text blocks to use PGML features. '
+		. 'Generally the conversion of much of the formatting and LaTeX is performed correctly. However, answer '
+		. 'blanks need attention. In any case, make sure to inspect the formatted code, and edit further or revert '
+		. 'if needed. Third, the code can be analyzed by the "PG Critic." This checks that the code does not use '
+		. 'old or deprecated features of PG and conforms to current best-practices in problem authoring, and offers '
+		. 'suggestions on how to fix issues that are found.') =%>
 	</dd>
 
 	<dt><%= maketext('Save') %></dt>
