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 to_str method #13

Closed
mintuhouse opened this issue Mar 31, 2016 · 10 comments · Fixed by #20
Closed

Add to_str method #13

mintuhouse opened this issue Mar 31, 2016 · 10 comments · Fixed by #20
Assignees
Milestone

Comments

@mintuhouse
Copy link

alias_method :to_str, :to_s in MaterialIcon Class
Rails safe_join calls to_str on objects passed before they are concatenated

@Angelmmiguel
Copy link
Owner

Hello @mintuhouse,

I check the code of safe_join helper:

def safe_join(array, sep=$,)
  sep = ERB::Util.unwrapped_html_escape(sep)

  array.flatten.map! { |i| ERB::Util.unwrapped_html_escape(i) }.join(sep).html_safe
end

In Rails 4.2, ERB::Util.unwrapped_html_escape doesn't call to to_str method. It's only perform a gsub in strings that not are marked as html_safe.

In Rails master ERB::Util.unwrapped_html_escape method is different, but I can't find any reference to to_str method.

Where do you see safe_join is calling to_str method?

@mintuhouse
Copy link
Author

Hi @Angelmmiguel
Sorry, I over looked. I got can't convert MaterialIcon to String (MaterialIcon#to_str gives MaterialIcon) in this line
Just realized that this safe_join is a custom implementation from the gem

I feel its still a good thing to have as [material_icon, some_view_helpers].join gives above error.

@Angelmmiguel
Copy link
Owner

No problem :). What version of Ruby are you using @mintuhouse?

@mintuhouse
Copy link
Author

2.1.8

@Angelmmiguel Angelmmiguel added this to the v2.2.1 milestone Mar 31, 2016
@Angelmmiguel
Copy link
Owner

Okey.

I find this interesting article about the difference between to_s and to_str methods. I think MaterialIcons meets the requirement to implement to_str, the purpose of the class is to be represented as String.

I will add this functionality for the next release of Material Icons gem. While next version is released, you can use material_icon.to_s in these situations because String object implements to_str method.

@mintuhouse
Copy link
Author

ok. Thanks @Angelmmiguel

@Angelmmiguel
Copy link
Owner

@mintuhouse I investigate the problem and it comes from the way to declare the icon name. I use a custom method_missing to set all non-defined methods as the icon to display:

# Undefined method will ref to the icon.
def method_missing(name)
   @icon = clear_icon(name)
    self
end

Array#join call to to_str method that is not defined in MaterialIcon. I think Ruby process this case and call to_s to return a String. MaterialIcon returns itself and join fails.

@Angelmmiguel Angelmmiguel added bug and removed feature labels Mar 31, 2016
@Angelmmiguel
Copy link
Owner

I think I will migrate this behaviour to an icon method because using method_missing is causing some problems.

@mintuhouse
Copy link
Author

But wouldn't that be a backward incompatible change (assuming we won't maintain complete list of allowed icons - in say .yml file)?

@Angelmmiguel
Copy link
Owner

Angelmmiguel commented Mar 31, 2016

Yes, the problem is that change breaks compatibility with older versions. That's the reason I haven't changed it yet :)

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

Successfully merging a pull request may close this issue.

2 participants