Improve documentation for `Object#to_s` and `#inspect` #9974

straight-shoota · 2020-11-26T00:35:43Z

This patch changes the method definitions as proposed in #9966 and adds some additional notes.

The part about ambigouity of collection members is removed because that's not generically relevant for #inspect but for individual implementations such as Array#inspect. So I added a short note about it there.
It does not need to be repeated for every collection type.

Resolves #9966

This patch changes the method definitions as proposed in crystal-lang#9966 and adds some additional notes. The part about ambigouity of collection members is removed because that's not generically relevant for `#inspect` but for individual implementations such as `Array#inspect`. So I added a short note about it there. It does not need to be repeated for every collection type.

src/object.cr

src/array.cr

src/object.cr

jhass · 2020-11-26T08:43:19Z

src/array.cr

@@ -1930,6 +1930,17 @@ class Array(T)
    self
  end

+  # Prints a nicely readable and concise string representation of this array


Suggested change

# Prints a nicely readable and concise string representation of this array

# Prints a readable and concise string representation of this array

Let's avoid subjective language :) Ditto all the other instances.

I think "nicely" fits very well here. Just "readable" doesn't cut it. Of course, it should be readable. But so does #inspect. This one should be a nice, informal representation.

src/array.cr

src/object.cr

jhass · 2020-11-26T09:00:56Z

src/object.cr

+  # identical environment) is an ideal representation. It doesn't need to be
+  # actual code that compiles, but a resemblance is recommended.
+  # When this is not possible (for example for non-printable internal state)
+  # another useful description will do.


Let's be honest, this is a lie. It's true for literal'ish values, but beyond that the representation never even tries represent valid code. The default implementations for Reference and Struct make a good example of my point here. You wouldn't want the inspect representation to be like SomeStruct.new(a: ..., b: ...) as that's noisy and a little weird.

So I'm not sure this is true for people that are not standard library/compiler developers like us. You don't work much with "custom" objects that are literals elsewhere.

Maybe the wording could be a bit relaxed, but in general I think it's a good idea to try to print Crystal expressions. That's concise and unambiguous. Struct#inspect actually isn't far from Crystal code.
Simple structs/classes which are just containers for a vew ivars are already covered by the respective default implementations. These notes are particularly relevant for more advanced types. For example custom collection types. They can actually use array-like or hash-like literals. HTTP::Params{"foo" => "bar"} would be a perfect inspect representation. Currently, it's more bulky: HTTP::Params(@raw_params={"foo" => ["bar"]}). That's an example from stdlib but you have such types in shards, too.

I'm also not entirely happy with this wording, but it's more of style than anything else. Perhaps we can pick a couple of (good) examples from the stdlib instead?

I can't add a suggestion to the entire block, so here it goes in plain 🤷

# Prints to *io* an unambiguous and information-rich string representation of this # object, typically intended for developers. # # It is similar to `#to_s(IO)`, but often provides more information. Ideally, it should # contain sufficient information to be able to recreate an object with the same value # (given an identical environment). # # For types that don't provide a custom implementation of this method, # default implementation delegates to `#to_s(IO)`. This said, it is advisable to # have an appropriate `#inspect` implementation on every type. Default # implementations are provided by `Struct#inspect` and `Reference#inspect`. # # `::p` and `::p!` use this method to print an object in `STDOUT`.

src/object.cr

sdogruyol

Thank you @straight-shoota 🙏

src/object.cr

beta-ziliani · 2022-12-02T14:44:59Z

src/object.cr

+  # identical environment) is an ideal representation. It doesn't need to be
+  # actual code that compiles, but a resemblance is recommended.
+  # When this is not possible (for example for non-printable internal state)
+  # another useful description will do.


I'm also not entirely happy with this wording, but it's more of style than anything else. Perhaps we can pick a couple of (good) examples from the stdlib instead?

beta-ziliani · 2022-12-02T14:47:33Z

src/object.cr

-  # Similar to `to_s(io)`, but usually appends more information
-  # about this object.
-  # See `#inspect`.
+  # `::p` and `::p!` use this method to present an object.


Is it necessary to have the :: here? doc doesn't resolve it correctly? More a curiosity than a request to change.

Suggested change

# `::p` and `::p!` use this method to present an object.

# `p` and `p!` use this method to present an object.

Yeah otherwise the docs generator wouldn't catch the reference to p.

Fryguy · 2022-12-02T15:05:34Z

src/array.cr

+  #
+  # The result resembles an array literal but it does not necessarily compile.
+  #
+  # Each element is presented using its `#inspect(io)` result to avoid ambiguity.


Should we mention avoiding circular references here? Or perhaps some other wording that explains the ...? The docs also serve for people writing overrides and I think catching circular references is important for those overriders to know about.

I think we only have to revisit this if/when we consider making exec_recursive a public API

Co-authored-by: Beta Ziliani <beta@manas.tech>

beta-ziliani

The comment for inspect was still a bit odd for me. I rewrote it. WDYT?

beta-ziliani · 2022-12-12T19:38:33Z

src/object.cr

+  # identical environment) is an ideal representation. It doesn't need to be
+  # actual code that compiles, but a resemblance is recommended.
+  # When this is not possible (for example for non-printable internal state)
+  # another useful description will do.


I can't add a suggestion to the entire block, so here it goes in plain 🤷

# Prints to *io* an unambiguous and information-rich string representation of this # object, typically intended for developers. # # It is similar to `#to_s(IO)`, but often provides more information. Ideally, it should # contain sufficient information to be able to recreate an object with the same value # (given an identical environment). # # For types that don't provide a custom implementation of this method, # default implementation delegates to `#to_s(IO)`. This said, it is advisable to # have an appropriate `#inspect` implementation on every type. Default # implementations are provided by `Struct#inspect` and `Reference#inspect`. # # `::p` and `::p!` use this method to print an object in `STDOUT`.

…9974) Co-authored-by: Beta Ziliani <beta@manas.tech>

straight-shoota added kind:docs topic:stdlib labels Nov 26, 2020

bcardiff reviewed Nov 26, 2020

View reviewed changes

src/object.cr Outdated Show resolved Hide resolved

src/array.cr Outdated Show resolved Hide resolved

src/object.cr Outdated Show resolved Hide resolved

jhass reviewed Nov 26, 2020

View reviewed changes

straight-shoota added 2 commits November 26, 2020 15:29

Improve wording

d774e2e

Improve wording for to_s usage

4133daa

Sija reviewed Dec 2, 2020

View reviewed changes

src/object.cr Outdated Show resolved Hide resolved

src/object.cr Outdated Show resolved Hide resolved

straight-shoota added 2 commits January 12, 2021 18:36

Merge branch 'master' into docs/to_s-inspect

add7ab3

crystal tool format

dd307e7

straight-shoota mentioned this pull request Mar 8, 2022

Improve UUID#inspect to return Crystal expression #11879

Open

sdogruyol approved these changes Nov 15, 2022

View reviewed changes

beta-ziliani reviewed Dec 2, 2022

View reviewed changes

Fryguy reviewed Dec 2, 2022

View reviewed changes

Apply suggestions from code review

30306a8

Co-authored-by: Beta Ziliani <beta@manas.tech>

straight-shoota self-assigned this Dec 8, 2022

beta-ziliani reviewed Dec 12, 2022

View reviewed changes

Apply suggestion from code review

b706407

beta-ziliani approved these changes Dec 14, 2022

View reviewed changes

beta-ziliani added this to the 1.7.0 milestone Dec 14, 2022

straight-shoota merged commit dab949e into crystal-lang:master Dec 15, 2022

straight-shoota deleted the docs/to_s-inspect branch December 15, 2022 12:25

carlhoerberg pushed a commit to carlhoerberg/crystal that referenced this pull request Dec 21, 2022

Improve documentation for Object#to_s and #inspect (crystal-lang#…

3d17ac9

…9974) Co-authored-by: Beta Ziliani <beta@manas.tech>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve documentation for `Object#to_s` and `#inspect` #9974

Improve documentation for `Object#to_s` and `#inspect` #9974

straight-shoota commented Nov 26, 2020

jhass Nov 26, 2020

straight-shoota Nov 26, 2020

jhass Nov 26, 2020

straight-shoota Nov 26, 2020

beta-ziliani Dec 2, 2022

beta-ziliani Dec 12, 2022

sdogruyol left a comment

beta-ziliani Dec 2, 2022

beta-ziliani Dec 2, 2022

straight-shoota Dec 8, 2022

Fryguy Dec 2, 2022

HertzDevil Dec 2, 2022

beta-ziliani left a comment

beta-ziliani Dec 12, 2022

	# Prints a nicely readable and concise string representation of this array
	# Prints a readable and concise string representation of this array

	# `::p` and `::p!` use this method to present an object.
	# `p` and `p!` use this method to present an object.

Improve documentation for Object#to_s and #inspect #9974

Improve documentation for Object#to_s and #inspect #9974

Conversation

straight-shoota commented Nov 26, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sdogruyol left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beta-ziliani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Improve documentation for `Object#to_s` and `#inspect` #9974

Improve documentation for `Object#to_s` and `#inspect` #9974