openwebwork/pg

Merged
merged 19 commits into from May 19, 2016

Conversation

Projects
None yet
3 participants
Member

dpvc reviewed Jan 23, 2016

 if ($known_units) {$options->{known_units} = $known_units; } my %Units = Units::evaluate_units($units,{fundamental_units => $fundamental_units, known_units =>$known_units});

dpvc Jan 23, 2016

Member

Does this want to use $options rather than the explicit hash. It looks like you are setting up $options for that, but never use them.

goehle Jan 23, 2016

Author Member

Good catch. I was adding and removing stuff for testing purposes and likely added the wrong thing back in.

goehle added some commits Jan 23, 2016

 remove extra code. 
 01b80c1 
 remove extra code v2 
 0ce48f4 
Member

dpvc commented Jan 23, 2016

 Just FYI, to check for leakage, you probably need to run the second problem several times, since you have to make sure it runs in the same child process as the original problem. Since the children are parceled out in various orders, you probably won't get the same child for the second problem on your first try. I'm sure you know that, but just in case anyone else it doing testing. I haven't had a chance to run tests by hand yet, but just looked through the code by hand. It looks good. I'll see if I can get some time this weekend to give it a try.

goehle added some commits Jan 27, 2016

 Merge branch 'develop' of https://github.com/openwebwork/pg into newunit 
 9477345 
 Initial commit for Selenium testing harness. 
 9208703 
 wrong place. 
 03be0a1 
 changes to the skel 
 8e1b875 
 Merge branch 'develop' of https://github.com/openwebwork/pg into newunit 
 ee18113 
 Merge branch 'newunit' of https://github.com/goehle/pg into newunit 
 5ed6074 
 Adding selenium tests. 
 556c142 

dpvc reviewed Jan 31, 2016

 @@ -0,0 +1,60 @@ #!/usr/bin/perl

dpvc Jan 31, 2016

Member

Did you mean to add these ~ files?

dpvc reviewed Jan 31, 2016

 # the hashes for these local copies to the NumberWithUnits package to use # for all of its stuff. %fundamental_units = %Units::fundamental_units; %known_units = %Units::known_units;

dpvc Jan 31, 2016

Member

It looks like these are setting global values in the main:: namespace. Should these be my variables?

dpvc reviewed Jan 31, 2016

 # the hashes for these local copies to the NumberWithUnits package to use # for all of its stuff. %fundamental_units = %Units::fundamental_units; %known_units = %Units::known_units;

Member

Same here.

goehle Jan 31, 2016

Author Member

I didn't mean to add the ~ files. They don't actually work right now, but
I had to update my personal repo so I could transfer between machines and
those changes showed up here. Its what I get for working on something in
consideration for pull requests. I'll get rid of them.

Its not always clear to me why some variables are local and some are global
in PG. For example %Units::known_units is available as a variable
directly. If I add my's should I also add accessors? Its not impossible
that someone would want to be able to see what units are available. If I
do leave them as global then I could also give them better names.

dpvc Jan 31, 2016 • edited

Member

Its not always clear to me why some variables are local and some are global in PG

Yes, I understand. I think much of that is left over from before PG used packages, and that was basically the only option.

If I add my's should I also add accessors?

The actually variables used are the my variables in Parcer::Legacy::ObjectWithUnits that you added in the first file at the top of the page. The ones here are only used to get copies of the ones from Units:: to pass to initializeUnits() in the line below. That is the routine that actually saves the values in the my variables that are used for the rest of the problem. So these definitely should be local variables, not global ones.

If you want to give access to the ones in Parser::Legacy::ObjectWithUnits, then that is a different question. I'm not sure it is necessary. The available units are the the ones in Units::known_units plus the ones defined in the problem, so there shouldn't be much question about that. I suppose other macro packages could define some that you didn't know about.

 Removed unwanted files and cleaned up variable decs. Tests still broken. 
 c02906d 
Member Author

goehle commented Jan 31, 2016

 Good point. I've made the suggested changes.
 Cleaned up and fixed tests. 
 f1c1cd9 
Member Author

goehle commented Feb 1, 2016

 I've cleaned up the unit tests so they work. You can try them out by also checking out the Automated Testing branch (openwebwork/webwork2#681) and getting Selenium set up. Then you can just run perl /opt/webwork/pg/t/Selenium/Tests/parserNumberWithUnit/parser.t  Again this is a bit of a proof of concept to see if these tests are useful. The basic idea is that you can keep a pg file in the testing folder and use the existing utilities to easy make a problem to test. Then you would use the Selenium IDE to generate some tests on the rendered version of the problem. In theory having these tests, and providing them with pull requests will speed things up, but it depends on if people use them and if they are robust enough.

goehle added some commits Feb 2, 2016

 Added ability to override testing uname and pwd. 
 9f65593 
 Removing tests. 
 a44fd11 
Member

mgage commented May 19, 2016

 I've verified that there is no leakage in this code but I have some suggestions before we pull it. For the file parserNumberWithUnits.pl how about changing it to: our %fundamental_units = %Units::fundamental_units; our %known_units = %Units::known_units; sub _parserNumberWithUnits_init { # We make copies of these hashes here because these copies will be unique to # the problem. The hashes in Units are shared between problems. We pass # the hashes for these local copies to the NumberWithUnits package to use # for all of its stuff. Parser::Legacy::ObjectWithUnits::initializeUnits(\%fundamental_units,\%known_units); # main::PG_restricted_eval('sub NumberWithUnits {Parser::Legacy::NumberWithUnits->new(@_)}'); } sub NumberWithUnits {Parser::Legacy::NumberWithUnits->new(@_)}; sub parserNumberWithUnits::fundamental_units { return \%fundamental_units; } sub parserNumberWithUnits::known_units { return \%known_units; } sub parserNumberWithUnits::add_unit { my $newUnit = shift; my$Units= Parser::Legacy::ObjectWithUnits::add_unit($newUnit->{name},$newUnit->{conversion}); return %$Units; }  the accessor's are simply so that we can grab the current value of %known_units and %fundamental_units as stored in parserNumberWithUnits.pl. %Units::known_units didn't update as items were added. The add_unit subroutine is so that you can add the units to the files environment directly without having to create an answer evaluator. This seemed more natural to me although there is no reason to remove the automatic adding of a unit to the environment when creating an evaluator. This is the test code I put in problems to determine which new units were defined: @check_these_units = qw(apple apples Spoon bear rabbit foobear); DEBUG_MESSAGE("checking for ", join(' ', @check_these_units),$BR); $string='';$fund = parserNumberWithUnits::fundamental_units(); $known = parserNumberWithUnits::known_units; foreach my$key (@check_these_units) { $string .="$key is a fundamental unit$BR" if exists($fund->{$key});$string .="$key is a known unit$BR" if exists($known->{$key}); $string.= "$key is an undefined unit $BR" unless exists($known->{$key}); } DEBUG_MESSAGE($string);  Change the formula file in the same way. Comments?
Member Author

goehle commented May 19, 2016

If you make a pull request against my feature branch I will merge it.

Cheers
Geoff
On May 18, 2016 10:39 PM, "Michael Gage" notifications@github.com wrote:

I've verified that there is no leakage in this code but I have some
suggestions before
we pull it. For the file
parserNumberWithUnits.pl how about changing it to:

our %fundamental_units = %Units::fundamental_units;
our %known_units = %Units::known_units;

sub _parserNumberWithUnits_init {

for all of its stuff.

Parser::Legacy::ObjectWithUnits::initializeUnits(%fundamental_units,%known_units);

main::PG_restricted_eval('sub NumberWithUnits {Parser::Legacy::NumberWithUnits->new(@_)}');

}
sub NumberWithUnits {Parser::Legacy::NumberWithUnits->new(@_)};
sub parserNumberWithUnits::fundamental_units {
return %fundamental_units;
}
sub parserNumberWithUnits::known_units {
return %known_units;
}
my $newUnit = shift; my$Units= Parser::Legacy::ObjectWithUnits::add_unit($newUnit->{name},$newUnit->{conversion});
return %$Units; } 1. the accessor's are simply so that we can grab the current value of %known_units and %fundamental_units as stored in parserNumberWithUnits.pl. %Units::known_units didn't update as items were added. 2. The add_unit subroutine is so that you can add the units to the files environment directly without having to create an answer evaluator. This seemed more natural to me although there is no reason to remove the automatic adding of a unit to the environment when creating an evaluator. This is the test code I put in problems to determine which new units were defined: @check_these_units = qw(apple apples Spoon bear rabbit foobear); DEBUG_MESSAGE("checking for ", join(' ', @check_these_units),$BR);
$string='';$fund = parserNumberWithUnits::fundamental_units();
$known = parserNumberWithUnits::known_units; foreach my$key (@check_these_units) {
$string .="$key is a fundamental unit$BR" if exists($fund->{$key});$string .="$key is a known unit$BR" if exists($known->{$key});
$string.= "$key is an undefined unit $BR" unless exists($known->{$key}); } DEBUG_MESSAGE($string);

Change the formula file in the same way. Comments?

You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#268 (comment)

 Create ability to add new units to problem before calling NumberWithU… 
…nits or FormulaWithUnits.

Simplify the way NumberWithUnits is called -- I don't believe the eval() is necessary.
Add accessor so the current value of units can be determined.
 61def70 
Member

mgage commented May 19, 2016

OK. That’s done. (It took three tries to submit a clean PR. :-) )

Take care,

Mike

On May 19, 2016, at 8:20 AM, Geoff Goehle notifications@github.com wrote:

If you make a pull request against my feature branch I will merge it.

Cheers
Geoff
On May 18, 2016 10:39 PM, "Michael Gage" notifications@github.com wrote:

I've verified that there is no leakage in this code but I have some
suggestions before
we pull it. For the file
parserNumberWithUnits.pl how about changing it to:

our %fundamental_units = %Units::fundamental_units;
our %known_units = %Units::known_units;

sub _parserNumberWithUnits_init {

for all of its stuff.

Parser::Legacy::ObjectWithUnits::initializeUnits(%fundamental_units,%known_units);

main::PG_restricted_eval('sub NumberWithUnits {Parser::Legacy::NumberWithUnits->new(@_)}');

}
sub NumberWithUnits {Parser::Legacy::NumberWithUnits->new(@_)};
sub parserNumberWithUnits::fundamental_units {
return %fundamental_units;
}
sub parserNumberWithUnits::known_units {
return %known_units;
}
my $newUnit = shift; my$Units= Parser::Legacy::ObjectWithUnits::add_unit($newUnit->{name},$newUnit->{conversion});
return %$Units; } 1. the accessor's are simply so that we can grab the current value of %known_units and %fundamental_units as stored in parserNumberWithUnits.pl. %Units::known_units didn't update as items were added. 2. The add_unit subroutine is so that you can add the units to the files environment directly without having to create an answer evaluator. This seemed more natural to me although there is no reason to remove the automatic adding of a unit to the environment when creating an evaluator. This is the test code I put in problems to determine which new units were defined: @check_these_units = qw(apple apples Spoon bear rabbit foobear); DEBUG_MESSAGE("checking for ", join(' ', @check_these_units),$BR);
$string='';$fund = parserNumberWithUnits::fundamental_units();
$known = parserNumberWithUnits::known_units; foreach my$key (@check_these_units) {
$string .="$key is a fundamental unit$BR" if exists($fund->{$key});$string .="$key is a known unit$BR" if exists($known->{$key});
$string.= "$key is an undefined unit $BR" unless exists($known->{$key}); } DEBUG_MESSAGE($string);

Change the formula file in the same way. Comments?

You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#268 (comment)

You are receiving this because you commented.
 Merge pull request #3 from mgage/newunit 
Create ability to add new units to problem before calling NumberWithU…
 1811a55