Rails core - Jan 2008 - Polymorphic URL helpers documentation and fixes

Yesterday I answered a question regarding polymorphic URL helpers on Core ML
and noticed that the module has no documentation. I''ve documented it
and
rewritten unit tests using Mocha. I also optimized some of the code slightly
(nothing major, though). The patch also includes two fixes by Geoff Buesing,
who has done awesome work in this area in the past.

http://dev.rubyonrails.org/ticket/10883

Check the documentation, please yell if anything is wrong:
http://dev.rubyonrails.org/attachment/ticket/10883/PolymorphicRoutes.html?format=raw

What''s still broken? (I decided not to fix everything in one go.)

   1. Hash argument: polymorphic_url(:id => @article). Test is commented
   out (like before).
   2. Should this work: polymorphic_url(@article, :format => :pdf) ?
   Currently it doesn''t. I''ve included a failing tests,
it''s commented out.

Open ActionPack tickets with "polymorphic" in title:
http://dev.rubyonrails.org/query?status=new&status=assigned&status=reopened&component=ActionPack&summary=%7Epolymorphic&order=priority

This one needs a discussion of its own:
http://dev.rubyonrails.org/ticket/9621

Thanks.
- Mislav

--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core-unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en
-~----------~----~----~----~------~----~------~--~---

On Jan 22, 2008 9:28 AM, Mislav Marohnić <mislav.marohnic@gmail.com>
wrote:
> Yesterday I answered a question regarding polymorphic URL helpers on Core
ML
> and noticed that the module has no documentation. I've documented it
and
> rewritten unit tests using Mocha. I also optimized some of the code
slightly
> (nothing major, though). The patch also includes two fixes by Geoff
Buesing,
> who has done awesome work in this area in the past.
>
> http://dev.rubyonrails.org/ticket/10883
>
> Check the documentation, please yell if anything is wrong:
>
>
http://dev.rubyonrails.org/attachment/ticket/10883/PolymorphicRoutes.html?format=raw
>
> What's still broken? (I decided not to fix everything in one go.)
> Hash argument: polymorphic_url(:id => @article). Test is commented out
(like
> before).
> Should this work: polymorphic_url(@article, :format => :pdf) ? Currently
it
> doesn't. I've included a failing tests, it's commented out.Open
ActionPack
I don't think it should work with these 'advanced' hash arguments. 
do
they do the right thing when passing through to the optimised named
route generators?  Do we want to support them going forwards?
> This one needs a discussion of its own:
> http://dev.rubyonrails.org/ticket/9621
I think that's just an abuse of inheritance and I'm not sure we could
do anything performant and reasonable to support that case, and the
case where AssetsController is meant to handle all the different
subclasses?


-- 
Cheers

Koz

--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core-unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en
-~----------~----~----~----~------~----~------~--~---

2008/1/22 Michael Koziarski <michael@koziarski.com>:
> > What''s still broken? (I decided not to fix everything in one
go.)
> > Hash argument: polymorphic_url(:id => @article). Test is commented
out
> (like
> > before).
> > Should this work: polymorphic_url(@article, :format => :pdf) ?
Currently
> it
> > doesn''t. I''ve included a failing tests,
it''s commented out.OpenActionPack
>
> I don''t think it should work with these
''advanced'' hash arguments.  do
> they do the right thing when passing through to the optimised named
> route generators?  Do we want to support them going forwards?

In my experience, formatted_foo_path(@foo, :pdf) and
formatted_foo_path(@foo, :format => :pdf) are identical. Still, my patch
only adds support for formatted_polymorphic_path([@foo, :pdf]), and
I''ve put
the hash argument up for discussion.
> http://dev.rubyonrails.org/ticket/9621
>
> I think that''s just an abuse of inheritance and I''m not
sure we could
> do anything performant and reasonable to support that case, and the
> case where AssetsController is meant to handle all the different
> subclasses?

I agree with such thinking; I''d only like these issues regarding
polymorphic
helpers resolved. Make the API concrete, reject features we don''t want
etc.
Thanks Koz

P.S. somehow you avoided to comment on my patch :D

--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core-unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en
-~----------~----~----~----~------~----~------~--~---

On 1/21/08, Michael Koziarski <michael@koziarski.com>
wrote:
>
> On Jan 22, 2008 9:28 AM, Mislav Marohnić <mislav.marohnic@gmail.com>
> wrote:
> >
> > Should this work: polymorphic_url(@article, :format => :pdf) ?
Currently
> it
> > doesn't. I've included a failing tests, it's commented
out.OpenActionPack
>
> I don't think it should work with these 'advanced' hash
arguments.  do
> they do the right thing when passing through to the optimised named
> route generators?  Do we want to support them going forwards?
>
>
Koz,
I've often wondered why (apart from specific implementation reasons) we need
a formatted_xxx helper to cater for formats at all.

Bearing in mind that the generated helper code (including optimization
stuff) could just look at the size of the hash and whether it's only member
is :format, is there a reason the generated helpers can't simply distinguish
between these signatures?

album_path(an_album)
album_path(an_album, :format => :xml)
albums_path()
albums_path(:format => :xml)

Somehow that seems easier on the eyes than:

album_path(an_album)
formatted_album_path(an_album, :xml)
albums_path()
formatted_albums_path(:xml)


I think it's because formatted_ seems to just add noise to the method name.
 The format isn't (to me) the most important thing, it's more of an
afterthought.  Keeping it's specification exclusively in the params mirrors
that imho.

Regards,
Trevor
-- 
--
Trevor Squires
http://somethinglearned.com

--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core-unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en
-~----------~----~----~----~------~----~------~--~---

I''m frequently in need of generating a formatted polymorphic url.
Currently, it''s done ugly because the formatted_polymorphic_url code
is broken (see ticket 8782 and Mislav''s #2 above).  I would like to
see that helper working-but if the job can be done with something
simpler, I''m all for it.  Trevor''s suggestion is intriguing
-leave the
"formatted" prefix off and use the signature to determine if
formatting is required.  That would be ideal.

On Jan 21, 8:35 pm, "Trevor Squires" <tre...@protocool.com>
wrote:
> On 1/21/08, Michael Koziarski <mich...@koziarski.com> wrote:
>
> > On Jan 22, 2008 9:28 AM, Mislav Marohnić
<mislav.maroh...@gmail.com>
> > wrote:
>
> > > Should this work: polymorphic_url(@article, :format => :pdf) ?
Currently
> > it
> > > doesn''t. I''ve included a failing tests,
it''s commented out.OpenActionPack
>
> > I don''t think it should work with these
''advanced'' hash arguments.  do
> > they do the right thing when passing through to the optimised named
> > route generators?  Do we want to support them going forwards?
>
> Koz,
> I''ve often wondered why (apart from specific implementation
reasons) we need
> a formatted_xxx helper to cater for formats at all.
>
> Bearing in mind that the generated helper code (including optimization
> stuff) could just look at the size of the hash and whether it''s
only member
> is :format, is there a reason the generated helpers can''t simply
distinguish
> between these signatures?
>
> album_path(an_album)
> album_path(an_album, :format => :xml)
> albums_path()
> albums_path(:format => :xml)
>
> Somehow that seems easier on the eyes than:
>
> album_path(an_album)
> formatted_album_path(an_album, :xml)
> albums_path()
> formatted_albums_path(:xml)
>
> I think it''s because formatted_ seems to just add noise to the
method name.
>  The format isn''t (to me) the most important thing, it''s
more of an
> afterthought.  Keeping it''s specification exclusively in the
params mirrors
> that imho.
>
> Regards,
> Trevor
> --
> --
> Trevor Squireshttp://somethinglearned.com
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core-unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en
-~----------~----~----~----~------~----~------~--~---

> I''ve often wondered why (apart from specific implementation
reasons) we need
> a formatted_xxx helper to cater for formats at all.
I don''t believe there''s a reason beyond the (considerable)
implementation specific reasons :).   Changing that with the current
routing implementation would probably be a little tricky without
making everyone generate urls like:

/albums/1.html
> I think it''s because formatted_ seems to just add noise to the
method name.
> The format isn''t (to me) the most important thing, it''s
more of an
> afterthought.  Keeping it''s specification exclusively in the
params mirrors
> that imho.
While it''s a little odd, I don''t see an easy migration path
that''s
suitable for a point release.

-- 
Cheers

Koz

--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core-unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en
-~----------~----~----~----~------~----~------~--~---

OK, so we''re keeping formatted_foo helpers for now. But what of my
patch?
http://dev.rubyonrails.org/ticket/10883

It fixes issues. It adds tons of documentation. It provides more test
coverage. Is there anything wrong with it? Anything that someone would like
to put up for discussion, or to have me change it? Thanks.

- Mislav

On Jan 25, 2008 3:31 AM, Michael Koziarski <michael@koziarski.com> wrote:
>
> > I''ve often wondered why (apart from specific implementation
reasons) we
> need
> > a formatted_xxx helper to cater for formats at all.
>
> I don''t believe there''s a reason beyond the
(considerable)
> implementation specific reasons :).   Changing that with the current
> routing implementation would probably be a little tricky without
> making everyone generate urls like:
>
> /albums/1.html
>
> > I think it''s because formatted_ seems to just add noise to
the method
> name.
> > The format isn''t (to me) the most important thing,
it''s more of an
> > afterthought.  Keeping it''s specification exclusively in the
params
> mirrors
> > that imho.
>
> While it''s a little odd, I don''t see an easy migration
path that''s
> suitable for a point release.
>
> --
> Cheers
>
> Koz
>
> >
>
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core-unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en
-~----------~----~----~----~------~----~------~--~---

> OK, so we''re keeping formatted_foo helpers for now. But what of my
patch?
>
> http://dev.rubyonrails.org/ticket/10883
>
> It fixes issues. It adds tons of documentation. It provides more test
> coverage. Is there anything wrong with it? Anything that someone would like
> to put up for discussion, or to have me change it? Thanks.
I''m happy with the documentation but not sure I buy the need for
#8720?  The intention of those helpers is that they automatically
figure out the right url to go to...  #8782 seems fine and the
documentation seems awesome.

-- 
Cheers

Koz

--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core-unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en
-~----------~----~----~----~------~----~------~--~---

On Jan 25, 2008 9:22 PM, Michael Koziarski <michael@koziarski.com> wrote:
>
> I''m happy with the documentation but not sure I buy the need for
> #8720?

Why match the API of polymorphic_path with polymorphic_url? Because there is
no need for these two to have different signatures, especially since
generated helpers for resources (foo_url vs. foo_path) have the same
parameters. I think consistency is key here, and since polymorphic_path
should be more used in general, it should take the parameters option.
Otherwise you would have to use polymophic_url with :routing_type => :path.

--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core-unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en
-~----------~----~----~----~------~----~------~--~---