Add Term::ReadLine::Perl5 support

[rocky]

The $isa_done stuff might be a little hoaky. Would like to not interfere with the Term::ReadLine::EditLine request but the two things do about the same thing and I don't see a way to avoid patches to not conflict.

[rafl]

I'm not quite sure why Term::ReadLine::Perl5 needs to be handled differently from all the other readline drivers.

Does your proposed change behave any different from the simpler 6a42fdc?

[rocky]

Yes, I think the behavior is different.

The reason for adding $isa_done is for the situation where when Term::ReadLine::Perl5 and Term::ReadLine::Gnu are both installed, but the environment variable PERL_RL specifies perl5.

Here, @ISA will have Term::ReadLine::GNU since that's tested first. And adding Term::ReadLine::GNU in @ISA will cause Term::ReadLine::GNU routines to get used when a plain Term::ReadLine is use'd.

[rafl]

I don't think your analysis is quite right:

$ perl -MTerm::ReadLine -MTerm::ReadLine::Gnu -e1 # i got ::Gnu installed
$ perl -MTerm::ReadLine -MTerm::ReadLine::Perl5 -e1 # and ::Perl5 as well
# now, with the patch proposed in https://github.com/rafl/term-readline/commit/6a42fdc6365b57d3fcfd3d4822cdb93e43339bf3
$ perl -Ilib -MTerm::ReadLine -E'say Term::ReadLine->new(q[foo])->ReadLine'
Term::ReadLine::Gnu
$ PERL_RL=perl5 perl -Ilib -MTerm::ReadLine -E'say Term::ReadLine->new(q[foo])->ReadLine'
Term::ReadLine::Perl5

So having both Gnu and Perl5 installed will be just fine. Having both Gnu and Perl5 loaded however, will chose Gnu. As far as I can tell, that was the original author's intention and it seems reasonable to me. If we wanna change that, we should discuss it separately from just adding support for Term::ReadLine::Perl5.

However, I can see how you could've gotten confused a bit - I know I was until I properly understood the following just now:

The usual way of using Term::ReadLine is to load it directly. During its loading process, it'll determine which readline driver to use by trying to load a bunch of them, and looking at the &readline symbol being defined by them.

The test you supplied with your patch, though, loads tries to load a particular driver, Term::ReadLine::Perl5, first in order to determine whether or not the test needs to be skipped.

Term::ReadLine::Perl5, however, is different from most other readline drivers I've seen in that it loads Term::ReadLine itself during its compile time. That can cause the following to happen:

• The user loads Term::ReadLine::Perl5, to check if it's available, for example
• As one of the very first things during its compilation, Term::ReadLine::Perl5 loads Term::ReadLine
• Term::ReadLine gets compiled and executed
• During its run time, it tries to find the readline driver to use
• If PERL_RL matches /\bperl5\b/, it will attempt to load Term::ReadLine::Perl5, which will succeed as it is, as far as require is concerned, already loaded and registered in %INC
• None of Term::ReadLine::Perl5's code is being executed during this require - the interpreter is still executing its require of Term::ReadLine during Term::ReadLine::Perl5's compile time.
• Term::ReadLine, after trying to load the best or the user specified readline driver, will go on to check for the existence of the &readline symbol in the various driver packages
• It will look at the &Term::ReadLine::Perl5::readline symbol, and determine it is not present. It's not there yet as the sub readline in that package hasn't been compiled yet - we're still compiling the use Term::ReadLine; statement in Term::ReadLine::Perl5
• Term::ReadLine, lacking any defined &readline symbols, will fall back to its stub implementation and the user will be sad.

Basically, things are being weird because there's a circular require involved.

We could fix this issue in a variety of ways:

• Load Term::ReadLine later during Term::ReadLine::Perl5's compile time
• Load Term::ReadLine during Term::ReadLine::Perl5's run time, as it's not ever using it during compile time anyway
• Prevent the loading of Term::ReadLine::Perl5 if Term::ReadLine isn't already loaded, similar to how ::Gnu does it
• Make the readline driver detection in Term::ReadLine lazier

Regardless of this issue, though, I still propose going ahead with merging 87dd145 as I can't really see any evidence for this change behaving different from the one you proposed and because the circular require issue that might cause some problems seems to be separate from supporting ::Perl5.

Please let me know if you're ok with that. If you aren't, it'd be really helpful to have a test or just a code snippet to demonstrate the problem you were trying to solve with the somewhat more complicated patch you proposed initially.

[rocky]

I don't think your analysis is quite right:

If by "analysis" you mean using the word "installed" when I should have written "loaded", well, okay.

As far as I can tell, that was the original author's intention

What leads you to believe this was the original author's intention as opposed to say non-consideration of the issue? Please elaborate.

and it seems reasonable to me.

Why is this "reasonable"? Please elaborate.

It strikes me as arbitrary, but not reasonable and here's why. It might have been at one time that Term::ReadLine::Gnu was more feature-rich than Term::ReadLine::Perl, but would that always be the case? And is it for the author/maintainer of Term::ReadLine to decide that rather than say the user of this module? I don't think so.

And if this is to be the behavior, shouldn't it be documented somewhere other than reading the code?

If one has two implementations loaded, which to me seems "reasonable", how can one influence which one Term::ReadLine will prefer? There already is that environment variable PERL_RL and that seems like what should be used.

The distinction of "installed" versus "loaded" and "compiled" makes my head explode.

... We could fix this issue in a variety of ways: ...
Load Term::ReadLine during Term::ReadLine::Perl5's run time, as it's not ever using it during compile time anyway

Ok. With commit a8ca9dd I have removed the load during compile time. And delayed the require until a "new" is done which should eliminate the cyclic require.

Prevent the loading of Term::ReadLine::Perl5 if Term::ReadLine isn't already loaded, similar to how ::Gnu does it

I don't like this option. Recall that the time between the first mention of this adding this into Term::ReadLine has been quite some time -- and it's still not resolved. So right now the only way for something like Devel::Trepan to try to use Term::ReadLine::Perl5.

Independent of that, why not allow loading of those other kinds of implementations?

Regardless of this issue, though, I still propose going ahead with merging 87dd145

Sure, it's better than nothing which is what's there now. But consider commit df7b274 which somewhat addresses the lack of describing what values for PERL_RL are currently permissible.

If you want me to open the documentation shortcomings in Term::ReadLine separately, let me know.

[rocky]

I was looking at this cyclic require issue yet again. I think the crux of the problem is that Term::ReadLine and Term::ReadLine::Stub are bundled into one file when they are different, albeit related, things.

How about splitting them into two files, e.g. Term/ReadLine.pm and Term/ReaLine/Stub.pm with one package namespace each? They can still live in the same overarching distribution package.

By doing this, ReadLine implementations like Term::ReadLine::Perl, Term::ReadLine::Gnu, and Term::ReadLine::Perl5 can choose to use Term::ReadLine::Stub without Term::ReadLine. And it appears that some of the stub functions is all they want.

Term::ReadLine would pull in Term::ReadLine::Stub as well, so those other packages aren't forced to change their use of Term::ReadLine to Term::ReadLine::Stub.

Finally, the reason those other packages don't seem to have a problem while Term::ReadLine::Perl5 does is that those other packages forgo "strict" and "warnings".