Convert to PGML: add the ability to parse loadMacros if the arguments are in a qw by pstaabp · Pull Request #1425 · openwebwork/pg

pstaabp · 2026-05-28T19:34:27Z

Add the ability to parse loadMacros if the arguments are in a qw block. Also, removes empty macros and duplicate macros.

In addition, if it appears that the problem is already in PGML mode, then return the file without changes.

This is in response to openwebwork/webwork2#2908

drgrice1 · 2026-06-01T19:14:42Z

There are some outlier cases that are doing some weird things with this. These cases probably are not worth putting much effort into, but they could happen, and are valid Perl, and this turns them into invalid Perl.

Either loadMacros(qw{PGstandard.pl MathObjects.pl}, 'draggableProof.pl'); or loadMacros(qw{PGstandard.pl MathObjects.pl draggableProof.pl},); becomes

loadMacros(qw{
	PGstandard.pl
	PGML.pl
	draggableProof.pl
	PGcourse.pl
');

The second case is probably more of an issue, because that is probably not that unlikely.

Closely related and more serious though is the following. loadMacros(qw{PGstandard.pl MathObjects.pl draggableProof.pl} ); becomes

loadMacros(qw{
	PGstandard.pl
	PGML.pl
	draggableProof.pl}
	PGcourse.pl
 );

This needs to handle the possibility of white space between the qw ending and the ending parenthesis.

pstaabp · 2026-06-09T16:02:18Z

This addresses the cases of @drgrice1.

I also tried to format the output on a single line if the input was on a single line and multiple lines if the input was that way.

drgrice1

This still needs some more work.

drgrice1 · 2026-06-20T01:09:18Z

+	# Check that the file is not already in PGML format by looking for PGML.pl in the loadMacros statement.
+	# and there are no BEGIN_TEXT, BEGIN_SOLUTION, etc. blocks.


There is something wrong with this comment. The first line ends with a period, and the second line starts with a lowercase "and".

drgrice1 · 2026-06-20T02:23:42Z

+			while (1) {
+				# Remove comments within loadMacros block (should we keep them?)
+				$row =~ s/#.*$//;
+				$macros .= $row;
+				last if ($row =~ /(.*)\);/);
+				++$num_macro_lines;
 				$row = shift @rows;
-				my @mrow = split(/#/, $row);
-				# This only adds the row if there is something relevent to the left of a #
-				$macros .= $mrow[0] if $mrow[0] !~ /^\s*$/;
 			}


This could potentially loop forever. I tried to convert the following to PGML, and that happened:

DOCUMENT(); loadMacros('PGstandard.pl', 'PGML.pl', 'parserMultiAnswer.pl', 'PGcourse.pl') BEGIN_TEXT Bad stuff END_TEXT

Yes, it is intentionally designed to make this loop forever, but someone using the PG problem editor should not be able to crash the server by trying to convert bad code. Note that the above code does not cause an infinite loop with the code for this in the PG-2.21 branch.

Good point. I'm wondering the best way to handle this situation. We could throw an error in this situation, but I don't want this to become a syntax checker.

To an extent, any code formatter needs to be a syntax checker. If the code it is trying to format is not valid, then it really cannot proceed. All of the code formatters such as perltidy and prettier work this way. Basically, what should happen is that if something invalid is encountered, then the formatter should just stop and give some error message.

Regardless, the point is that the infinite loop cannot happen. So this pull request is stopped until this is a definite no go until this is resolved.

I have a fix for this now.

block. Also, removes empty macros and duplicate macros. In addition, if it appears that the problem is already in PGML mode, then return the file without changes.

in loadMacros

pstaabp · 2026-07-07T18:45:01Z

This now checks for some parsing of the loadMacros call.

Additionally, this returns a hash for the output with any parsing errors. I will post a PR for WeBWorK to handle the errors in the PGeditor.

The command line works with no additional code.

Also, return a hash of the converted code and any errors.

pstaabp · 2026-07-08T11:14:52Z

Note: there is now updated PGEditor code in openwebwork/webwork2#3046 that handles errors in the PG problem editor.

drgrice1

Something is definitely not working. The example given by @somiaj in issue #2908 doesn't even work. It gives the error PGML conversion errors: The loadMacros command cannot be parsed. In fact, every single file I have tested, regardless of any content that it has, gives that error unless PGML.pl happens to be on the same line as the loadMacros call.

The bin/convert-to-pgml.pl script is really poorly written. There should at the very least be a help option using pod2usage from Pod::Usage. If the script is called with no arguments which are required, then instead of dying with the message that arguments must have a list of files the help should be shown. Also, the SYNOPSIS section in the POD should be updated to show the usual help that is shown with the help option with the pod2usage method that shows the way the script should be called with all options described.

Also, the ConvertToPGML.pm module exports symbols by default. That shouldn't happen. @EXPORT_OK should be used instead of @EXPORT.

I must not have reviewed this well before when you first added this. Those things should not have been allowed.

drgrice1 · 2026-07-09T15:11:09Z

+	my $pg_source = $path->slurp;
+	my $result    = convertToPGML($pg_source);
+	if (ref($result) eq 'HASH' && $result->{errors}) {
+		warn "Error parsing $filename. " . $result->{errors};


Add a newline to the end of the warn so that the line number in the convert-to-pgml.pl script is not reported. That does not pertain to the error, and so should not be shown. So change this to

Suggested change

warn "Error parsing $filename. " . $result->{errors};

warn "Error parsing $filename. " . $result->{errors} . "\n";

drgrice1 · 2026-07-09T15:22:56Z

+	# and there are no BEGIN_TEXT, BEGIN_SOLUTION, etc. blocks.
+
+	return { pgmlCode => $pg_source }
+		if ($pg_source =~ /loadMacros\((.*)PGML\.pl(.*)\)/m && $pg_source !~ /BEGIN_(TEXT|HINT|SOLUTION)/);


This extremely naive regular expression check is just not going to work. First, since you do not include the s flag on the regular expression, the . character does not match newlines. So unless PGML.pl is literally on the same line as the loadMacros call, this check will not find it. So this will catch something like loadMacros( #PGML.pl) which it shouldn't, but it will not catch something like

loadMacros(qw{ PGstandard.pl PGML.pl });

Note that just adding the s flag won't fix this either because then it would still catch the false positive noted above, but would also catch something like

loadMacros('PGstandard.pl'); # (This does not use PGML.pl) In addition, the second regular expression check will also catch many incorrect things. For example, if someone mentions `BEGIN_TEXT` in a comment in the file, but does not use it.

pstaabp changed the title ~~Add the ability to parse loadMacros if the arguments are in a qw~~ Convert to PGML: add the ability to parse loadMacros if the arguments are in a qw May 29, 2026

somiaj reviewed May 31, 2026

View reviewed changes

Comment thread lib/WeBWorK/PG/ConvertToPGML.pm Outdated

pstaabp force-pushed the pgml-convert-improvements branch from 1db3b80 to ba081b2 Compare June 9, 2026 15:50

drgrice1 reviewed Jun 20, 2026

View reviewed changes

pstaabp added 3 commits July 7, 2026 14:39

Add the ability to parse loadMacros if the arguments are in a qw

ded55a3

block. Also, removes empty macros and duplicate macros. In addition, if it appears that the problem is already in PGML mode, then return the file without changes.

remove debug statements

8fee5f9

Handle cases where qw blocks are mixed with strings in ' or "

a14eed6

in loadMacros

pstaabp force-pushed the pgml-convert-improvements branch from ba081b2 to d8f5572 Compare July 7, 2026 18:42

Add some error checking for loadMacros on the source

e8114b7

Also, return a hash of the converted code and any errors.

pstaabp force-pushed the pgml-convert-improvements branch from d8f5572 to e8114b7 Compare July 8, 2026 11:01

pstaabp mentioned this pull request Jul 8, 2026

Add error handling in the pg problem editor openwebwork/webwork2#3046

Open

drgrice1 requested changes Jul 9, 2026

View reviewed changes

		# Check that the file is not already in PGML format by looking for PGML.pl in the loadMacros statement.
		# and there are no BEGIN_TEXT, BEGIN_SOLUTION, etc. blocks.

	warn "Error parsing $filename. " . $result->{errors};
	warn "Error parsing $filename. " . $result->{errors} . "\n";

Uh oh!

Uh oh!

Conversation

pstaabp commented May 28, 2026

Uh oh!

Uh oh!

drgrice1 commented Jun 1, 2026

Uh oh!

pstaabp commented Jun 9, 2026

Uh oh!

drgrice1 left a comment

Choose a reason for hiding this comment

Uh oh!

drgrice1 Jun 20, 2026

Choose a reason for hiding this comment

Uh oh!

drgrice1 Jun 20, 2026

Choose a reason for hiding this comment

Uh oh!

pstaabp Jun 25, 2026

Choose a reason for hiding this comment

Uh oh!

drgrice1 Jun 25, 2026

Choose a reason for hiding this comment

Uh oh!

pstaabp Jul 7, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pstaabp commented Jul 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pstaabp commented Jul 8, 2026

Uh oh!

drgrice1 left a comment

Choose a reason for hiding this comment

Uh oh!

drgrice1 Jul 9, 2026

Choose a reason for hiding this comment

Uh oh!

drgrice1 Jul 9, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

pstaabp commented Jul 7, 2026 •

edited

Loading