Make Fraction contexts as extensions so you can add fractions to other compatible contexts.

[dpvc]

This PR modifies the contextFraction.pl file to use the context-extension framework, making it possible to add fractions to any context. It still creates the original four fraction contexts, but also provides context::Fraction::extending() that can be used to add fractions to other contexts. E.g., you could use context::Fraction::extending("Matrix") to get a context in which fractions can be entered in matrices.

This should be backward compatible with the current contextFraction.pl from an author's perspective, except for the following:

• The strictMultiplication and strictMinus flags have been removed, as they are no longer needed (the strict handling is done by extending the LimitedNumeric context, rather than reimplementing the strict behavior).
• The handling of the internal information about integer values, negative values, and fractions has been modified. In the past, the object's class was modified to return INTEGER, MINUS and FRACTION, but now that fractions can be attached to arbitrary contexts, we don't want to modify the class of numbers, since that could affect how the other context operates. Instead these values are stored internally in private data within the typeRef objects.

It appears that at least one problem (Contrib/PCC/BasicAlgebra/LinearEquationApplications/Ratio20.pg) subclasses the LimitedFraction class and uses the original class values, so would need to be modified to work with the new contexts. I have renamed the original contextFraction.pl as legacyFraction.pl, for use in cases like this, though you all can decide if that is necessary or not.

Finally, I modified the t/contexts/fraction.t file to include the needed Parser classes for the new contextFractions.pl. Of course, that test file doesn't test most of the functionality, and needs many more tests to fully test the various options.

This PR includes the contextExtensions.pl file, even though I made a separate PR for it. Because that branch isn't in the openwebwork/pg repository, I can't target this PR to that branch. When that PR is merged, the contextExtensions.pl file should disappear from here.

[dpvc]

PS, because this contextFraction.pl file is substantially different from the original, it may help to just view the file directly, rather than try to navigate the diff.

[drgrice1]

I am seeing an issue with the following MWE:

DOCUMENT();
loadMacros('PGstandard.pl', 'PGML.pl', 'contextFraction.pl', 'PGcourse.pl');

Context('Fraction');
$ans = Compute('x/7');

BEGIN_PGML
[`[$ans] =`] [_]{$ans}{10}
END_PGML
ENDDOCUMENT();

That works with the develop branch, but gives an error with this pull request.

[dpvc]

OK, I forgot to include the cmp_defaults method. I was pretty sure I had tested that, but I guess I misremembered (probably remembered testing the units contexts instead). I've pushed the fix.

[drgrice1]

That fixes the issue with that example. Thanks.

I am seeing another issue when different contexts are used as in the following:

DOCUMENT();
loadMacros('PGstandard.pl', 'PGML.pl', 'contextFraction.pl', 'PGcourse.pl');
Context('Fraction');
$fractionAns = Compute('x/7');
Context('Numeric');
$numericAns = Compute("3");
BEGIN_PGML
[`[$fractionAns] =`] [_]{$fractionAns}{10}

[`[$numericAns] = `] [_]{$numericAns}{10}
END_PGML
ENDDOCUMENT();

[drgrice1]

Note that if either answer is removed from that problem it works. Also, if the fraction does not have x in it, then there is no problem.

[drgrice1]

You latest changes fix the issues! Thanks.

[dpvc]

You latest changes fix the issues! Thanks.

Yes, I was still doing some more changes before writing up the details.

It turns out that changing the context was what was causing the problem. Although MathObjects contain a pointer to the context that created them, there are places where new objects are created via their new() or make() methods where they are passed the context, since the $self reference is the actual class name, not an instance, in those cases. That means those new() and make() calls don't have $self->context set to the context where the original object was created, but rather it defaults to the current context. Since the extension data is stored in the original object's context, if the context has changed, these new() and make() calls need to use the passed context in order to get the extension information. This wasn't taken into account in the context::Extensions::Super class, and so I had to include passing the context to these in some instances in order to get it to work when the context has been changed, as in your example.

I need to update the units PR to accommodate those changes as well.

You may not like the perltidy format

I don't object to it in principle, but I think some of the internal alignment make things harder to read, and some of the spacing is unnecessary. For example, just because two consecutive lines have equal signs or sufficient if statements doesn't mean those equal signs or ifs should be lined up. For example, this

    $x = $context->Package("Formula")->new($context, $x)->eval if !ref($x) && $x =~ m!/!;
    $x = $x->eval                                              if @_ == 0  && Value::classMatch($x, 'Fraction');

is nonsense in my opinion. It is much harder to see the second line as being a conditional when the if is so far away from the assignment. I also don't care for the extra spacing within braces, parens, and brackets for single-line expressions, like the spaces added in the brackets here, though I know that is a popular approach:

    my $class = join('::', (split(/::/, ref($self) || $self))[ 0, 1 ]);

That was the source of my commit comment. I frequently end up shaking my head at some of the changes it makes. But I understand the desire to have consistency, and you all can decide on the formatting you want to use. I may still grumble about it at times, however.

[drgrice1]

I also don't like some of the formatting at times. In some cases we may be able to improve things by tweaking the settings. At least you can do that, unlike prettier for JavaScript, typescript, etc. I just know that some of the webwork and PG code used to be really untradable with excessively inconsistent indentation in particular.

[drgrice1]

The particular alignment issue you mentioned is certainly not the nicest in appearance, I agree.

[dpvc]

I agree with you about the consistency for indenting, line breaks, and continuation lines; that is a big win. It is the internal alignment that I find so unsettling. It seems to me that internal alignment should be used to indicate a meaningful structural connection between two lines, but most of the internal alignment that I see is meaningless, like the one I illustrated above (and I did choose it because of its particularly grievous nature, though most aren't that bad). That has several consequences. First, it makes things harder to read because the extra spacing interferes with reading the individual line, and one is incorrectly lead to believe there is a connection to a previous or following line, and it takes time to recognize that that is not the case. Second, because that is so frequently not the case, it trains you to ignore the alignment, as it is generally is meaningless. That means you have watered down the meaning of alignment, so that when it truly is important, you are less likely to recognize it among the sea of other nonsense alignments. For the most part, I think internal alignment should be relatively rare, and reserved for cases where a connection needs to be emphasized. I'm happy with alignment of => arrows in multi-line hash definitions, and even alignment of equal signs during the initialization of variables can be useful, but not every consecutive equal sign should be aligned with a previous one, and aligning && in the if statements like done in my example above is generally unnecessary and should only be done when the structure of the two really is connected. Similarly, aligning entries in two consecutive array literals is probably not called for except when the two arrays are connected in some way.

My own preference would be to have the authors do the internal alignment that is important (since they are the ones that understand which ones have meaning, whereas perltidy clearly doesn't), and have a style guide that indicates the ones that you expect to make, and request that authors abide by that before merging if they don't. By forcing such alignments, you may be prevention poor coders from making bad decisions, but you are also forcing other bad decisions, and preventing good coders from providing meaningful alignments (and actively undoing the meaningful ones they include). That seems to be to be guaranteeing a mediocre output; certainly not as bad as it was, but not as good as it could be.

Again, I realize it is up to the currently active programming community (of which I am not longer a member) to decide these issues, and I will abide by the choices that are made. But I reserve the right to make snarky commit comments now and again. :-)

[dpvc]

I should point out that my recent changes to handle your cortex-switching example included a minor change to lib/Value.pm. It turns out that

my $symbol = shift || '';

would perform stringification on the first argument in order to do the || operation, and that caused a problem in the Value::isContext() function in some situations. So this has been updated to use // instead of ||. That should not be a controversial change.

[dpvc]

PS, I meant to thank you for the careful testing. Very helpful!