Skip to content
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

remove BEGIN_ONE_COLUMN and END_ONE_COLUMN #854

Merged
merged 7 commits into from
Jun 28, 2023

Conversation

Alex-Jordan
Copy link
Contributor

If openwebwork/webwork2#2049 is merged, then this should be considered on the PG side. If openwebwork/webwork2#2049 is merged, there is no more defining of \nocolumns by the hardcopy theme, and then these macros no longer serve a purpose. Worse, they cause problems when they are used with a two-column theme. This will make their presence in a .pg file issue a perl warning that they are undefined. Again if openwebwork/webwork2#2049 is merged, then removing these from any pg files is appropriate.

Of course, comments welcome if I'm overlooking something.

@pstaabp
Copy link
Sponsor Member

pstaabp commented Jun 19, 2023

There seem to be over 200 problems in the OPL with these there. Most are CUNY "problems" that seem to be set headers rather than problems--we can probably find someone to clean these up. If this goes in, we should have a Library PR that removes these.

@drgrice1
Copy link
Sponsor Member

I think that rather than removing these methods, they should be made stubs for backwards compatibility.

@drgrice1
Copy link
Sponsor Member

I guess I don't really mean stubs, but methods that just output the empty string.

@Alex-Jordan
Copy link
Contributor Author

Is there a way to make it work smoothly but also communicate to an instructor that it's dead? In general I'd like to remove things from problems that people use as templates/models, but are not helpful anymore. For example, OPL problems seem to still have TEXT(beginproblem());. That kind of thing (and $BEGIN_ONE_COLUMN) mislead people to think they need to include that.

@drgrice1
Copy link
Sponsor Member

@Alex-Jordan: You could change the BEGIN_ONE_COLUMN method to be

sub BEGIN_ONE_COLUMN {
	warn '$BEGIN_ONE_COLUMN is deprecated. ' . "Please remove the usage of this variable.\n" if $envir{isInstructor};
}

Although, this warning would not be particularly prevalent. Students would never see it, and even instructors would only see it in certain places. I currently set it up so that warnings are not propagated from the rendering subprocess when set headers are rendered for html. However, the warning would be displayed for instructors editing the set header in the PG problem editor, and would be shown when an instructor generates a hardcopy for any set using the header.

You could do the same with END_ONE_COLUMN. Although, since these are usually used together, that would result in both warnings shown. Again only in those cases mentioned with the isInstructor conditional.

@Alex-Jordan
Copy link
Contributor Author

I made those changes, and now this diff just has the stubs with warnings.

@pstaabp
Copy link
Sponsor Member

pstaabp commented Jun 22, 2023

@Alex-Jordan I think (but can't find where) you mentioned updating the modelCourse, which has a number of cases with BEGIN_ONE_COLUMN.

@drgrice1
Copy link
Sponsor Member

Those were removed in openwebwork/webwork2#2049 (or openwebwork/webwork2#2032 which the former included).

@drgrice1
Copy link
Sponsor Member

I guess there are still instances of those in set0, setDemo, setMAAtutorial, setOrientation.

@pstaabp
Copy link
Sponsor Member

pstaabp commented Jun 22, 2023

I got rid of all cases of BEGIN_ONE_COLUMN in my course, but see warning on every problem now when viewing as an instructor. Here's the relevant part of the warning:

from within main::_PGbasicmacros_init called at line 185 of [PG]/lib/PGloadfiles.pm

Does line 84 and 142 call BEGIN_ONE_COLUMN()? I didn't think this actually makes a function call.

@Alex-Jordan
Copy link
Contributor Author

I would like to completely replace the four sets that come with the distribution. But for now I can just remove these macros from their header files.

My school is having a network outage and I can't use my dev server at the moment. When I can, I will look into the warnings you are getting.

@drgrice1
Copy link
Sponsor Member

Ahh, so my suggestion won't work. The problem is that the $BEGIN_ONE_COLUMN variable is defined when PGbasicmacros.pl is loaded and _PGBasicmacros_init is called by calling the BEGIN_ONE_COLUMN method. So you will get the warnings anytime a problem is rendered. Sorry, my suggestion was not a good one.

@Alex-Jordan
Copy link
Contributor Author

It was worth a try though. I will update the branch when I get access back to my dev server. And I'll remove the macros from the four repo sets in a separate PR.

Copy link
Sponsor Member

@pstaabp pstaabp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good now.

Copy link
Sponsor Member

@drgrice1 drgrice1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good. Now that they are marked as deprecated we can remove them in twenty years or so!

@pstaabp pstaabp merged commit 87258d3 into openwebwork:PG-2.18 Jun 28, 2023
2 checks passed
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.

3 participants