[jifty-devel] Template::Declare Import Bug

[David E. Wheeler]

Hey All,

I think I've run into a bug in Template::Declare's importing behavior.
Consider:

package Foo;
use base 'Template::Declare';
use Template::Tags;
template foo => sub { say 'hello' };

package Bar;
use base Template::Declare;
use Template::Tags;
import Foo under '/foo';

package Baz;
use base Template::Declare;
use Template::Tags;
import Foo under '/otherfoo';

In this example, I've imported the Foo templates under /foo in the Bar
package, and under /otherfoo in the Baz package. The problem is this:
what should `Foo->path_for('foo')` return? Because I've imported its
templates into two classes, it currently returns the path is was last
imported into (/otherfoo/foo). But that only makes sense if you're
using it from package Baz, not if you're using it from package /Bar.
What if you want to use it from Bar? What if Bar is the root, but Baz
is not?

Is `path_for` something that's actually used for something? If so,
maybe the package name should be passed to it?

Foo->path_for('Bar'); # returns /foo/foo
Foo->path_for('Baz'); # returns /otherfoo/foo

Thoughts?

Best,

David

[David E. Wheeler]

Post by David E. Wheeler
Is `path_for` something that's actually used for something? If so,
maybe the package name should be passed to it?
Foo->path_for('Bar'); # returns /foo/foo
Foo->path_for('Baz'); # returns /otherfoo/foo
Thoughts?

FYI, I commented out path_for() and the two tests for it, and all
tests passed. So it doesn't seem necessary to the internals. Is it
used externally, e.g., in Jifty? Is it necessary? Could it be
deprecated?

Best,

David

[Jesse Vincent]

Post by David E. Wheeler

Post by David E. Wheeler
Is `path_for` something that's actually used for something? If so,
maybe the package name should be passed to it?
Foo->path_for('Bar'); # returns /foo/foo
Foo->path_for('Baz'); # returns /otherfoo/foo
Thoughts?

FYI, I commented out path_for() and the two tests for it, and all
tests passed. So it doesn't seem necessary to the internals. Is it
used externally, e.g., in Jifty? Is it necessary? Could it be
deprecated?

This _is_ a feature Jifty uses.

Shawn, could you look at how we might be able to improve or replace it?

Post by David E. Wheeler
Best,
David
_______________________________________________
jifty-devel mailing list
http://lists.jifty.org/cgi-bin/mailman/listinfo/jifty-devel

--

[David E. Wheeler]

Post by Jesse Vincent
This _is_ a feature Jifty uses.
Shawn, could you look at how we might be able to improve or replace it?

A backwards-compatible change would be to allow the template class
name to be passed, as I specified. If no argument is passed, it could
fall back on the current behavior, which is simply the last class into
which it was imported.

I should look at how aliasing affects this, too…

David

[Jesse Vincent]

Post by David E. Wheeler

Post by Jesse Vincent
This _is_ a feature Jifty uses.
Shawn, could you look at how we might be able to improve or replace it?

A backwards-compatible change would be to allow the template class
name to be passed, as I specified. If no argument is passed, it could
fall back on the current behavior, which is simply the last class into
which it was imported.
I should look at how aliasing affects this, too…

*nod* That sounds reasonable

[David E. Wheeler]

Post by Jesse Vincent

Post by David E. Wheeler
A backwards-compatible change would be to allow the template class
name to be passed, as I specified. If no argument is passed, it could
fall back on the current behavior, which is simply the last class into
which it was imported.
I should look at how aliasing affects this, too…

*nod* That sounds reasonable

Crap. I just realized that this won't work either, because you can
import the same templates into the same class with a different prefix.

package Wifty::UI;
import_templates Wifty::Mixin under '/mixout';
import_templates Wifty::Mixin under '/mixin';

So even if I called `path_for('hello', 'Wifty::UI')`, what would it
return?

Best,

David

[Ruslan Zakirov]

Post by David E. Wheeler

Post by Jesse Vincent

Post by David E. Wheeler
A backwards-compatible change would be to allow the template class
name to be passed, as I specified. If no argument is passed, it could
fall back on the current behavior, which is simply the last class into
which it was imported.
I should look at how aliasing affects this, too…

*nod* That sounds reasonable

Crap. I just realized that this won't work either, because you can
import the same templates into the same class with a different prefix.
    package Wifty::UI;
    import_templates Wifty::Mixin under '/mixout';
    import_templates Wifty::Mixin under '/mixin';
So even if I called `path_for('hello', 'Wifty::UI')`, what would it
return?

David, I took a look at path_for while we were discussing TD. It's
just a function we specific use case and may be it should be hidden
unless there requirement in similar general functionality.

Post by David E. Wheeler
Best,
David
_______________________________________________
jifty-devel mailing list
http://lists.jifty.org/cgi-bin/mailman/listinfo/jifty-devel

[David E. Wheeler]

Post by Ruslan Zakirov

Post by David E. Wheeler
So even if I called `path_for('hello', 'Wifty::UI')`, what would it
return?

David, I took a look at path_for while we were discussing TD. It's
just a function we specific use case and may be it should be hidden
unless there requirement in similar general functionality.

Yes, talking to Sartak about it on IRC, we agreed that it should be
documented as deprecated. If it wasn't for the fact that it was
documented in the past, I would suggest not documenting it at all. But
alas, that's probably not the best option.

http://search.cpan.org/~sartak/Template-Declare-0.40/lib/Template/Declare.pm#import_templates

Best,

David

[David E. Wheeler]

Post by David E. Wheeler
Yes, talking to Sartak about it on IRC, we agreed that it should be
documented as deprecated. If it wasn't for the fact that it was
documented in the past, I would suggest not documenting it at all. But
alas, that's probably not the best option.
http://search.cpan.org/~sartak/Template-Declare-0.40/lib/Template/Declare.pm#import_templates
Note that this will only work for the last class into which you
imported the template. This method is, therefore, deprecated.

Best,

David

[Jesse Vincent]

Post by David E. Wheeler

Post by Ruslan Zakirov

Post by David E. Wheeler
So even if I called `path_for('hello', 'Wifty::UI')`, what would it
return?

David, I took a look at path_for while we were discussing TD. It's
just a function we specific use case and may be it should be hidden
unless there requirement in similar general functionality.

Yes, talking to Sartak about it on IRC, we agreed that it should be
documented as deprecated. If it wasn't for the fact that it was
documented in the past, I would suggest not documenting it at all. But
alas, that's probably not the best option.

The specific usage I added this for, I _think_, was Jifty's CRUD
templates when subclassed inside an application. But yeah, we'll need
to figure out a sane replacement for it. Doccing it as deprecated with
the understanding that it needs to hang around until Jifty has moved on
is the right way forward.

Thanks again for all your work to improve Template::Declare, David. It's
hugely appreciated.

Best,
Jesse

[David E. Wheeler]

Post by Jesse Vincent
The specific usage I added this for, I _think_, was Jifty's CRUD
templates when subclassed inside an application. But yeah, we'll need
to figure out a sane replacement for it. Doccing it as deprecated with
the understanding that it needs to hang around until Jifty has moved on
is the right way forward.

There are three pieces of data:

* The template name.
* The class into which it was mixed
* The path under which it was mixed

So you'd need something where you specified the name of the class you
mixed into, as well as the path. I could see something like this

package Wifty::UI;
mix Wifty::Stuff under '/stuff';
mix Wifty::Stuff under 'dupe';

package Wifty::Other;
mix Wifty::Stuff under 'here';

The data for this should be stored for Wifty::Stuff as:

$mixed_into = {
'Wifty::UI' => [ '/stuff', 'dupe' ],
'Wfity::Other => [ 'here' ]
};

Then you'd just need a method (or methods) to interrogate it. Would
that work for your CRUD?

Best,

David