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
Contributor

@javan javan commented Nov 14, 2013

Example:

class User<ActiveRecord:: Base
  to_param : name
end

user = User.find_by(name: 'Fancy Pants')
user.id       # => 123
user.to_param # => "123-fancy-pants"

@rafaelfranca
Copy link
Member

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

Now I got what the PR is doing. 😰

@dhh
Copy link
Member

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}"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need tests to this truncate logic

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
@xymbol
Copy link
Contributor

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 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 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

Yes sounds important the possibility to use an option

@kuldeepaggarwal
Copy link
Contributor

Please refer this #12967.

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

Successfully merging this pull request may close these issues.

None yet

7 participants