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

Add aliases for Angstrom to Units.pm #762

Merged
merged 6 commits into from
Feb 2, 2023

Conversation

duffee
Copy link
Contributor

@duffee duffee commented Jan 6, 2023

The SI symbol for the ampere is "A", which PG uses to mean Angstrom (unit of length).

To facilitate the migration of the Angstrom unit from "A" to "Angstrom" (or something else), this change adds in two aliases so that documentation and code can be updated before any breaking changes are made.

I put this PR up here for discussion about #743 . I've added both upper and lower case for the unit for comparison, although I think it would be better to stick with one or the other.

@pstaabp
Copy link
Member

pstaabp commented Jan 26, 2023

@duffee We think that we should probably standardize to SI units. Can you include in this PR that A will now be ampere and we'll keep both "Angstrom" and "angstrom"?

@Alex-Jordan
Copy link
Contributor

Note that Å is an available unicode character for angstrom, and could maybe be in play here.

@duffee
Copy link
Contributor Author

duffee commented Jan 30, 2023

@pstaabp I've changed A to from angstrom to ampere, leaving amp as the fundamental unit. I'll prepare a separate PR for the documentation changes required in webwork2.
I see from wikipedia that the official unit is the lowercase, but I'll leave the uppercase version if you think there's benefit.

I think it's a bad idea to add the Å because unicode is hard generally, but I admire Alex's courage, so it's in!

@Alex-Jordan
Copy link
Contributor

I know what you mean, but PG should be supporting it now. Already (if you enable it in localOverrides.conf) you can use things like π, ∞, and ≤ in answers. Students often do, copy-pasting a character from somewhere into an input field. We have all that enabled on our server and it works. But to my knowledge this would be the first non-ascii character in the units.

@pstaabp
Copy link
Member

pstaabp commented Jan 31, 2023

I notice that the unit tests fail. Oddly, when I checkout this branch and run the test, I'm getting different errors. This might be because currently the tests are run on GitHub without the -I lib option to prove and to get these to run correctly, we need prove -I lib t.

Also, I just wrote a simple problem to test:

DOCUMENT();
loadMacros("PGstandard.pl","MathObjects.pl","PGML.pl","parserNumberWithUnits.pl");

TEXT(beginproblem());

$a = NumberWithUnits("10 Angstrom");

BEGIN_PGML

10 Angstrom = [__]{$a}

10 angstrom = [__]{$a}

10 [@ chr(0x00C5) @] = [__]{$a}

[`10^{-9} m`] = [__]{$a}
END_PGML

ENDDOCUMENT();

This was to test the Å as a unit. It seems like it works, however either PGML or perl or the browser doesn't seem to display 'Å' correctly. This is a screenshot of what I'm getting on this. Before submitting:
image

and after submitting:
image

@drgrice1
Copy link
Member

The workflow does not need to be changed. @duffee needs to work into the way that unit tests for PG are supposed to be set up and quit deleting the use lib "$ENV{PG_ROOT}/lib"; line. This pull request will not be approved and merged until those are put back.

@drgrice1
Copy link
Member

@pstaabp: By the way, you can also just do prove -rl t instead of prove -r -I lib t.

@drgrice1
Copy link
Member

drgrice1 commented Jan 31, 2023

Ah, sorry @duffee. You didn't delete those lines. The lines just are not present in this pull request. You need to rebase onto the develop branch or merge the develop branch into this one (whichever you prefer).

@drgrice1
Copy link
Member

There are still some failing tests in t/units/basic_parser.t even after rebasing or merging.

t/units/basic_parser.t Outdated Show resolved Hide resolved
@pstaabp
Copy link
Member

pstaabp commented Jan 31, 2023

I just added:

[@ helpLink("units") @]*

to the sample problem which opens the help for units. I'm seeing the old version:

image

even though the help code clearly changed from "A" to "angstrom". Is this a caching issue? I just flushed my cache and checked a rarely used browser and still see "A" as the basic unit.

@drgrice1
Copy link
Member

@pstaabp: That is because the file that @duffee changed is not used for helpLink. The file that needs to be updated to fix that is pg/htdocs/helpFiles/Entering-Units.html.

@drgrice1
Copy link
Member

AnswerFormatHelp.pl is another instance of a macro that should be deprecated.

@Alex-Jordan
Copy link
Contributor

AnswerFormatHelp.pl is another instance of a macro that should be deprecated.

+1

@pstaabp
Copy link
Member

pstaabp commented Jan 31, 2023

Using the proper format of AnswerFormatHelp worked fine I used:

[@ AnswerFormatHelp('units') @]*

and it does show the updated file. Do mean deprecate the helpLink subroutine?

@drgrice1
Copy link
Member

@pstaabp: What? ... I mean what??????? What are you saying?

@drgrice1
Copy link
Member

I definitely mean deprecate the AnswerFormatHelp.pl macro that is incorrect, obsolete, and replaced with better functionality by helpLink.

@pstaabp
Copy link
Member

pstaabp commented Jan 31, 2023

I definitely mean deprecate the AnswerFormatHelp.pl macro that is incorrect, obsolete, and replaced with better functionality by helpLink.

Looking at the AnswerFormatHelp.pl code, yes. I agree. It's in 8000+ OPL/Contrib problems. That'll be some fun changes.

So the update to the unit help should be in htdocs/HelpFiles/units.html.

@drgrice1
Copy link
Member

Removing it from OPL and Contrib problems may not be fun, but it doesn't change the fact that the macro is obsolete. What should be done is the macro updated to call helpLink, and marked as deprecated. Newly written problems should not use it.

@drgrice1
Copy link
Member

drgrice1 commented Feb 1, 2023

@duffee: The tests that are failing after develop is merged into this branch are tests that I added that use angstroms. They use the old A unit, and need to be updated to using the new angstrom (or Å) unit.

The SI symbol for the ampere is "A", which PG uses to mean Angstrom.
To facilitate the migration of the Angstrom unit
from "A" to "Angstrom" (or something else), this change adds in
two aliases so that documentation and code can be updated
before a breaking change is made.
This is a breaking change for problems that use
A as angstrom.

Add in the unicode character Å \x{00C5} to Units.pm
as an alias for angstrom.

Add test for A as ampere and allow unit names to match a unicode character
Make the help file consistent with the code change
A is amp not angstrom
Rebase onto develop and fix tests that use A for angstrom
@drgrice1
Copy link
Member

drgrice1 commented Feb 1, 2023

I forgot to mention the file htdocs/helpFiles/Units.html which will also need to be updated. In fact its contents should be identical to those of htdocs/helpFiles/Entering-Units.html. The PGbasicmacros.html helpLink method actually uses the Units.html file. To be honest, I don't know why both files exist. They both used to be in the webwork2/htdocs/helpFiles location, and I copied both of them over when the help files were moved here.

Needs to be the same as Entering-Units.html
@duffee
Copy link
Contributor Author

duffee commented Feb 2, 2023

Looks like the duplication is also the case for Syntax.html and IntervalNotation.html. I suppose you've already considered symlinks. Duplication sucks, but I don't have a proven solution at my fingertips.

Changes made to Units.html as requested.

@drgrice1
Copy link
Member

drgrice1 commented Feb 2, 2023

Yeah, those three are duplicated. I did think about using symlinks. That would work fine. Although, I really would like to delete the duplicates. I just need to figure out if there really is a need for them. It comes down to the question of does anyone directly use the help files by URL, or are the files only used by their helpLink alias? If no one uses them directly, then they can be removed.

The PDE-notation.html file does not have an helpLink alias. So the only way it can be used is by direct URL. So at least one file must be used that way (or no one uses it).

@duffee
Copy link
Contributor Author

duffee commented Feb 2, 2023

After finding and changing all the files in the repo that use those links, a decent deprecation strategy would be to change the file to note that the URL has been deprecated, instructions on how to update the link in the user's source and provide a link to click through to the surviving page. That should catch anyone who's bookmarked the page and after a year delete the file and listen for the 404's.

@drgrice1
Copy link
Member

drgrice1 commented Feb 2, 2023

We don't need to worry about someone bookmarking these. They are not served as complete pages. They are embedded html. So we really only need to worry about authors using the files directly by URL, rather than by the helpLink alias.

@pstaabp
Copy link
Member

pstaabp commented Feb 2, 2023

This looks good now and even the Å symbol seems to be working and displaying correctly.

image

@pstaabp pstaabp merged commit 0d56c6e into openwebwork:develop Feb 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants