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

to_str vs to_s #569

Closed
joelmoss opened this issue Apr 25, 2023 · 21 comments · Fixed by #629
Closed

to_str vs to_s #569

joelmoss opened this issue Apr 25, 2023 · 21 comments · Fixed by #629
Assignees
Milestone

Comments

@joelmoss
Copy link
Contributor

So at https://github.com/phlex-ruby/phlex/blob/main/lib/phlex/sgml.rb#L422 to_str is used, but not all objects respond to that method. Think using to_s is better.

As an example, ActiveSupport::TimeWithZone does not implement to_str, but does to_s, resulting in an exception when using a ActiveSupport::TimeWithZone instance as an attribute...

time(datetime: record.updated_at) { record.updated_at.to_fs :short }

undefined method 'to_str' for Wed, 23 May 2018 14:59:48.000000000 UTC +00:00:ActiveSupport::TimeWithZone

@joeldrapper
Copy link
Collaborator

joeldrapper commented Apr 25, 2023

Hey @joelmoss, this was intentional because to_s is usually used to represent an object for inspection. All kinds of objects respond to to_s, sometimes in unexpected ways — even things like Enumerators and Data objects.

For this use-case, you could define format_object in your base ApplicationComponent to handle ActiveSupport::TimeWithZone and format it with your preferred strftime representation.

def format_object(object)
  case object
  when ActiveSupport::TimeWithZone
    object.strftime("%H:%M on %d %B, %Y")
  else
    super
  end
end

This method is designed to be overridden like this with rich object formatters.

@joeldrapper
Copy link
Collaborator

Sorry, I got this wrong. The format_object method is only called by Phlex for text output, but I realised you're talking about attribute values.

@joelmoss
Copy link
Contributor Author

Yeah, I'm talking about attributes.

@joeldrapper
Copy link
Collaborator

joeldrapper commented Apr 25, 2023

Regarding attributes, I can see why this is frustrating, but on balance, I do think it's better to use to_str rather than to_s. So many objects respond to to_s in ways that are not designed for public output. If the objects can really be coerced to strings without any additional thought or parameters, they should respond to to_str.

In the case of TimeWithZone, there's no universal way to represent date-time-with-timezones as strings, so they should probably be converted into strings with strftime, which importantly takes formatting parameters.

The same is true of other rich objects such as Money. If it's a data attribute to be consumed by JavaScript, you probably want to represent Money as an integer of cents (e.g. 1000); if it's for display somewhere, you probably want a formatted string (e.g. $10.00). Phlex can't know this, so it's better to error until you specify.

One final example is ActiveRecord objects and other models: if I have an model (e.g. user), it responds to to_s with:

"#<User:0x000000012733b378>"

It could have been represented like this:

'#<User id: 6, email: "joel@drapper.me", role: "admin", first_name: "Joel", last_name: "Drapper">'

Or perhaps as an identifier:

6

Or a global identifier:

"gid://example/User/6"

Or a URL:

"/users/6"

Or JSON:

{ 
  "id" : 6,
  "email" : "joel@drapper.me",
  "role" : "admin",
  "first_name" : "Joel",
  "last_name" : "Drapper"
}

Or a name:

"Joel Drapper"

I don’t think there's a sensible default for many objects besides the currently supported String, Symbol, Integer, Float, true, false, nil, to_str-able, Arrays of the above types and Hashes with String|Symbol keys and values of the above types.

@joelmoss
Copy link
Contributor Author

That's actually really interesting, as it seems we both have a different view on #to_s and what it is generally used for. It seems that you would use it for technical reasons, whereas I really only ever use it for end-user display purposes, or as you put it, for public output.

For example in pretty much every model or class I have, I would define a to_s method that would return a user friendly way of identifying the object. So a User model with a name method/attribute would be returned from, or simply aliased to to_s. Then anything else would use a more specific method, such as to_json, to_url, etc.

But the real issue here - at least for me - is that #to_s is more universally used than to_str in Ruby land. So it makes more sense to use that

@joeldrapper
Copy link
Collaborator

joeldrapper commented Apr 25, 2023

There are a few articles on this topic:

Essentially, every Ruby class (with the exception of BasicObject) implements to_s, either as the default object inspect representation or with a specialised implementation. to_str, on the other hand, is usually only implemented by objects that can be coerced into a string without data loss, or where it makes sense for the object to be treated as if it were itself a string.

I think this is the interface we're looking for.

With a TimeWithZone attribute, I would define a private method on the component to format it into a string.

def template
  div(data_date: date)
end

def date = @date.strftime("%H:%M on %d %B, %Y")

With your own POROs, it may make sense to just implement to_str. For example, if you have a simple value object like UserID, it may be appropriate for that to be to_str-able.

UserID = Data.define(:value) do
  def to_str = case value
    in Integer then value.to_s
    in String then value
  end
end

@joelmoss
Copy link
Contributor Author

joelmoss commented Aug 15, 2023

Sorry to bring this up again, but this keeps cropping up over and over for me, and having to implement #to_str on every object is becoming a real pain.

It's even more frustrating that only String in the standard library implements #to_str, and even that is an alias to #to_s. It wouldn't be too bad if most implemented it, but hardly any do.

While I understand the reasonings behind preferring to_str, in reality very little code uses it.

Is there some sort of compromise we can find here?

@joeldrapper
Copy link
Collaborator

We could allow text to call to_s on the object. I don’t like the idea of using to_s for implicit text (from the block return) because every object in ruby implements to_s, including things like Enumerators.

@joelmoss
Copy link
Contributor Author

I agree with not using it for implicit text, so I think some sort of convention is needed so as to avoid me having to implement #to_str everywhere.

We could allow text to call to_s on the object

Could show an example?

@joeldrapper
Copy link
Collaborator

Could show an example?

What I mean is this wouldn't call to_s on the object.

h1 { object }

But this would

h1 { text object }

@joeldrapper joeldrapper reopened this Aug 29, 2023
@joelmoss
Copy link
Contributor Author

Yeah, but I'm talking about attributes remember 😏

@joeldrapper
Copy link
Collaborator

🤦‍♂️ Oh god, I keep doing this.

I guess it makes sense to call to_s on objects passed as attributes, though I’d prefer an interface that allows you to have an object that can be coerced into other valid attribute types such as true or false. For example, it could try to_phlex_attribute_value first and then fall back to to_s.

@joelmoss
Copy link
Contributor Author

Yeah I like that idea.

I'm guessing that would be best implemented at https://github.com/phlex-ruby/phlex/blob/main/lib/phlex/sgml.rb#L422

But that currently calls #to_str. So do we try to_phlex_attribute_value, to_s, to_str (in that order)?

@joeldrapper
Copy link
Collaborator

I’d prefer to try to_str before to_s, but it's probably better for performance to call to_s first since — let's be honest — almost no object implements to_str. 😅

@wdiechmann
Copy link

"upgrading" (the geese are there to signal that it certainly does not feel so) Phlex now hands me

ActionView::Template::Error (undefined method `to_str' for Location:Class

first I went hunting some of my own 'darlings' until I realized - when trying out endpoints not touched since like forever, that this upgrade really has "paid off"

  • SVG now works with a block [only] (not a biggie but it cheated me off a good 20min)
  • text is now plain (neither a biggie - but another 10-15min)
  • attributes on Phlex inherited classes now somehow has their to_s/to_str all screwed up
  • turbo streaming suddenly stopped working
  • probably more

I'm (as you may remember) a huge fan of Phlex - the DSL, the bang and all the fizz - but you might put up a (not so small) sign warning current users:

Road bump ahead

√ src % bundle list
Gems included by the bundle:
  * actioncable (7.0.7.2)
  *...8<...
  * phlex (1.8.1)
  * phlex-rails (1.0.0)
  * ...8<...

@joeldrapper
Copy link
Collaborator

joeldrapper commented Sep 7, 2023

Thanks for the feedback @wdiechmann. All of these changes were made after very careful consideration and documented in the release notes.

Turbo stream shouldn't have stopped working though. Could you share a bit more about that in a new discussion thread? We actually managed to get some changes into turbo-rails itself that makes it work even better with Phlex views and other renderable objects like ViewComponent components.

Also, I’m sorry the upgrade wasted 35 minutes of your time. I’ve probably spent over eighty thousand minutes working on phlex and phlex-rails so far so hopefully on the whole it's saved you a little time too.

@wdiechmann
Copy link

I understand that my 'ventilating discomfort' came out all wrong - apologies!

I'll hurry to the release notes while I enjoy all the time I've saved using Phlex!

If you ever feel adventurous please ping me - I've got 1-2 places for you to have a well-deserved R/R 😄

guess I'll stay with 1.3.2 for the time being though...

@joeldrapper
Copy link
Collaborator

@joelmoss want me to assign this issue to you or shall I take it? Happy either way.

@wdiechmann
Copy link

I'll be very happy to volunteer testing this issue in a 'real' project

@joelmoss
Copy link
Contributor Author

@joelmoss want me to assign this issue to you or shall I take it? Happy either way.

I was planning on grabbing it. Just need to find the time. 😕

@joeldrapper joeldrapper added this to the 1.10 milestone Dec 6, 2023
@joeldrapper joeldrapper modified the milestones: 1.10, 2.0 Jan 29, 2024
@joelmoss
Copy link
Contributor Author

joelmoss commented Jan 29, 2024

So it turns out that when trying to_phlex_attribute_value, to_s, to_str in order, the to_str will never get called, because Object implements to_s. Meaning every object at the very least respond to to_s.

So we either switch the order to to_phlex_attribute_value, to_str, to_s, or simply don't try to_str.

Thoughts?

FYI: draft PR #629

joeldrapper added a commit that referenced this issue Jan 30, 2024
Fixes #569 

Attribute values can be any object that responds to
`to_phlex_attribute_value` or `to_str`, falling back to `to_s` if
neither is defined.

Signed-off-by: Joel Drapper <joel@drapper.me>
Co-authored-by: Joel Drapper <joel@drapper.me>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

3 participants