Stas Bekman wrote:
Geoffrey Young wrote:


I should have noticed this before, but all the mod_perl foo in TestConfig really belongs in TestConfigPerl (including the stuff that was there before this flurry of commits :)

it's too late for 1.05 at this point (which I hope to release on monday or tuesday) but right afterward I'm going to clean up a bit and remove all traces of mod_perl from TestConfig.




Why? This code was there all the time and it has nothing to do with mod_perl and needed for any other perl project. Your quite shows like it wasn't added, but it wasn't, you have removed the - lines, which have shown where it existed before.



I'm not arguing about whether it was there before and you were just following suit. this isn't necessarily about the changes you just made.


ok

sorry you got the wrong impression :)


what I'm saying is that we have the TestConfigPerl class for configuring mod_perl specific widgets and TestConfig should probably be as non-httpd-core-module neutral as we can make it.

>

so, stuff like IS_MOD_PERL_2, IS_MOD_PERL_2_BUILD, and PerlRequire shouldn't be there conceptually. it just makes it harder to extend and maintain when there isn't a nice separation :)


Sure, so instead of:

  if (IS_MOD_PERL_2_BUILD || $ENV{APACHE_TEST_LIVE_DEV}) {

it should then say:

  if ($self->should_include_live_dev) {

which can be subclassed in TestConfigPerl with IS_MOD_PERL_2_BUILD moving there. so the superclass (TestConfig) goes:

  sub should_include_live_dev { return $ENV{APACHE_TEST_LIVE_DEV} ? 1 : 0 }

and TestConfigPerl:

   sub should_include_live_dev {
      return IS_MOD_PERL_2_BUILD || shift->SUPER::should_include_live_dev;
   }

does this sound reasonable?

yup, that's pretty much what I had in mind. there are a few other places where the fix might not be that simple, but we've both got the same idea at least :)


that TestConfigPerl isn't a real subclass might pose some issues, but we might be able to fix that as well :)

anyway, this is all after the release - it doesn't make sense to change it now that you've gotten the lib thing working. we'll sort it all out in a few days.

--Geoff



Reply via email to