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

[Text normalizer] Title, description, and keywords can be String-like objects #183

Merged
merged 2 commits into from
Nov 16, 2018

Conversation

kpumuk
Copy link
Owner

@kpumuk kpumuk commented Nov 16, 2018

Description

This PR is an extension of #175, adding support for #to_str to titles and keywords.

Closes #174

@kpumuk kpumuk merged commit bbdf86a into master Nov 16, 2018
kpumuk added a commit that referenced this pull request Nov 16, 2018
[Text normalizer] Title, description, and keywords can be String-like objects
@kpumuk kpumuk deleted the dmytro/to_str branch November 16, 2018 18:02
@codyrobbins
Copy link

I think it would make sense to change this to use to_s instead of to_str. I just started using this gem and was surprised to find that I couldn’t pass an Active Record model that implements to_s to title. In the meantime I’m just explicitly calling to_s but it would be much more elegant if that wasn’t needed. My understanding is that to_str is really only implemented on classes that for all intents and purposes are strings, which is quite uncommon.

By the way, while I’m writing, I just wanted to let you know that I love this gem and thank you for writing it!

@kpumuk
Copy link
Owner Author

kpumuk commented May 20, 2019

The choice to use to_str was deliberate and thought through.

In ruby, to_s returns a string representation of an object, and is supposed to be used explicitly (in fact, it is never used implicitly by internal methods). For example,

ruby-2.5.1 >> 'donuts: ' + 5
TypeError (no implicit conversion of Integer into String)

On the other hand, to_str defines an implicit conversion, e.g. it says "this object behaves like a string", and that is exactly what the library needs – it expects strings to render as metadata. And since this object is used in a context where we expect string, we chose to use implicit conversion, and so to_str.

ruby-2.5.1 >> Integer.alias_method(:to_str, :to_s)
→ Integer < Numeric
ruby-2.5.1 >> 'donuts: ' + 5
→ "donuts: 5"

Using explicit to_s is unsafe, as it might expose internal data as metadata by accident. If you plan to pass ActiveRecord objects to meta-tags gems, the easiest way is to simply alias an existing column to to_str:

class User < ApplicationRecord
  alias_method :to_str, :email
end

@codyrobbins
Copy link

Thanks for the quick reply! In actual practice, I disagree that to_s is used explicitly—and I think that’s especially true in the context of Rails, which this gem is targeting. Rails implicitly calls to_s all over the place, especially in views, and this gem is largely a set of view helpers. Just as an example, you don’t typically do something like this:

<%= (1 + 1).to_s %>

Methods like puts and Ruby features like string interpolation also implicitly invoke to_s.

On the other hand, I think your example of defining to_str on an Active Record model is inherently unsafe—to_str is intended for classes that are effectively interchangeable with strings in all circumstances, and that is definitely not the case with an Active Record model. I’m not sure the exact consequences of doing so in all circumstances which is what makes me wary about it.

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.

I have another gem initializing the variable @page_description
2 participants