Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add AR::Base.to_param for convenient "pretty" URLs derived from a model's attribute or method #12891

Merged
merged 1 commit into from Nov 14, 2013

Conversation

@javan
Copy link
Member

@javan javan commented Nov 14, 2013

No description provided.

…el's attribute or method.
@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Nov 14, 2013

Thank you but we can't change the Rails default and this is already possible to do in your application. We don't need this on Rails

dhh added a commit that referenced this pull request Nov 14, 2013
Add AR::Base.to_param for convenient "pretty" URLs derived from a model's attribute or method
@dhh dhh merged commit a23bf6f into rails:master Nov 14, 2013
@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Nov 14, 2013

Now I got what the PR is doing. 😰

@dhh
Copy link
Member

@dhh dhh commented Nov 14, 2013

It's all opt-in, all good :)

def to_param(method_name)
define_method :to_param do
if (default = super()) && (result = send(method_name).to_s).present?
"#{default}-#{result.truncate(20, separator: /\s/, omission: nil).parameterize}"

This comment has been minimized.

@rafaelfranca

rafaelfranca Nov 14, 2013
Member

We need tests to this truncate logic

javan added a commit to javan/rails that referenced this pull request Nov 14, 2013
* Fix incorrectly named tests
* Restore Object#to_param behavior
* Ensure param is derived from a squished and truncated string
rafaelfranca added a commit that referenced this pull request Nov 14, 2013
Addendum to #12891
@xymbol
Copy link
Contributor

@xymbol xymbol commented Dec 18, 2013

Hmm, a bit astonished to find this merged to master "as is". This implementation ignores all prior art on the subject, for example, FriendlyId. The code seems pretty expensive to generate, say, hundreds of resource urls for an index page. What's the need for uncached interpolation, truncation, parameterization, etc.? Yes, it's optional but, it's being offered as a convenience for people to use.

@dhh
Copy link
Member

@dhh dhh commented Dec 18, 2013

FriendlyId is great but far too complex for something default to fit in Rails. This is a great simple solution that'll be a nice upgrade for the 80% case. For when you want to go for the last 20%, external gems can fill the gap.

If you have some performance benchmarks showing this approach is an issue, do post along with a patch to speed it up. But the core functionality is solid as is.

On Dec 18, 2013, at 7:06, Adrian Mugnolo notifications@github.com wrote:

Hmm, a bit astonished to find this merged to master "as is". This implementation ignores all prior art on the subject, for example, FriendlyId. The code seems pretty expensive to generate, say, hundreds of resource urls for an index page. What's the need for uncached interpolation, truncation, parameterization, etc.? Yes, it's optional but, it's being offered as a convenience for people to use.


Reply to this email directly or view it on GitHub.

@rafbm
Copy link
Contributor

@rafbm rafbm commented Dec 21, 2013

The one thing that feels wrong to me in this addition is the hard 20 passed to truncate. I think a max_length option would bring a nice flexibility.

class User < ActiveRecord::Base
  to_param :name, max_length: 40
end

Using 20 as the default length seems a bit short to me too (think blog post slugs), but I guess this really depends on your use case.

@Fire-Dragon-DoL
Copy link
Contributor

@Fire-Dragon-DoL Fire-Dragon-DoL commented Feb 21, 2014

Yes sounds important the possibility to use an option

@kuldeepaggarwal
Copy link
Contributor

@kuldeepaggarwal kuldeepaggarwal commented Feb 21, 2014

Please refer this #12967.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.