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

Substitue #to_s for Dashboard#title_attribute #5

Merged
merged 1 commit into from
Apr 17, 2015
Merged

Substitue #to_s for Dashboard#title_attribute #5

merged 1 commit into from
Apr 17, 2015

Conversation

c-lliope
Copy link
Contributor

title_attribute was being used to display a link to each resource
throughout the dashboards.

It was acting as a kind of unique identifier, but was restrictive in
several ways:

  • The title_attribute had to correspond with a method on the model.
    It couldn't be computed from several methods.
  • Only the current dashboard could look up the correct method for
    displaying a resource. This causes problems for relationships like the
    BelongsToAdapter and HasManyAdapter, which need to know how to display
    other objects.

Moving to #to_s is good in several ways:

  • It makes representing an object super easy, with a single consistent
    method to call on each object.
  • It encourages developers to write good #to_s methods on each of
    their objects.

And bad in several ways:

  • If #to_s is not defined on a model, it defaults to the object's ID.
    This is not tied in any way to the database, so an object's identifier
    wouldn't be consistent throughout the dashboard.
  • If the user hasn't defined #to_s on a model, it doesn't noisily fail
    by default. If we wanted to noisily fail, we'd have to check against
    the default #to_s string and raise an error.

To fix the bad effects, there's room for a separate gem that generates
better default #to_s methods for objects, by trying for attributes
like name or title, and defaulting to id if those are not present.

@@ -13,7 +13,7 @@ def attribute_names
end

def page_title
resource.public_send(dashboard.title_attribute)
resource.to_s
Copy link
Collaborator

Choose a reason for hiding this comment

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

I like this approach - super clean! 👍

@jayroh
Copy link
Collaborator

jayroh commented Apr 16, 2015

lgtm boss!

`title_attribute` was being used to display a link to each resource
throughout the dashboards.

It was acting as a kind of unique identifier, but was restrictive in
several ways:

- The title_attribute had to correspond with a method on the model.
  It couldn't be computed from several methods.
- Only the current dashboard could look up the correct method for
  displaying a resource. This causes problems for relationships like the
  BelongsToAdapter and HasManyAdapter, which need to know how to display
  other objects.

Moving to #to_s is good in several ways:

- It makes representing an object super easy, with a single consistent
  method to call on each object.
- It encourages developers to write good `#to_s` methods on each of
  their objects.

And bad in several ways:

- If `#to_s` is not defined on a model, it defaults to the object's ID.
  This is not tied in any way to the database, so an object's identifier
  wouldn't be consistent throughout the dashboard.
- If the user hasn't defined `#to_s` on a model, it doesn't noisily fail
  by default. If we wanted to noisily fail, we'd have to check against
  the default `#to_s` string and raise an error.

To fix the bad effects, there's room for a separate gem that generates
better default `#to_s` methods for objects, by trying for attributes
like `name` or `title`, and defaulting to `id` if those are not present.
@c-lliope c-lliope merged commit a4f4640 into master Apr 17, 2015
@c-lliope c-lliope deleted the to_s branch April 17, 2015 01:55
jordan-brough referenced this pull request in jordan-brough/administrate Jan 20, 2022
* Remove jquery ujs (#1)

* Update HasMany show collection partial path (#2)

Built on thoughtbot#522

* Enable selectize for select and audience fields (#3 and #5)

* Enable selectize for voucher types field (#6)

* Use and test Ruby 3.0

Co-authored-by: Nic Hippenmeyer <nic@rinsed.co>
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.

2 participants