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

Use T#to_s instead of T#inspect for Array#to_s #5632

Closed
wants to merge 1 commit into from

Conversation

bew
Copy link
Contributor

@bew bew commented Jan 23, 2018

  • [1_i64].to_s gave: [1_i64]
  • [1_i64].inspect gave: [1_i64]

It was because Array#inspect used Array#to_s, with uses T#inspect instead of T#to_s.

Now Array#to_s uses T#to_s & Array#inspect uses T#inspect:

  • [1_i64].to_s should give: [1]
  • [1_i64].inspect should give: [1_i64]

This 'correct' to_s/inspect use could also be ported to Tuple, NamedTuple, Set, Hash, Range, and some others, but at least Array is done. I think it is the more important as it is usually used to print it's elements (not only for console debugging purpose).

@asterite
Copy link
Member

Note that this behavior was copied from Ruby, so maybe there's a reason to it. But if we change this, we should change Hash too and check other places too.

@asterite
Copy link
Member

I'd rather revert suffixes in numbers. They are too noisy.

@RX14
Copy link
Contributor

RX14 commented Jan 23, 2018

So someone came saying they depended on the to_s output format of array and suddenly we have to changer things? Why on earth aren't we just saying "don't depend on to_s/inspect on array".

@asterite
Copy link
Member

I don't know, I can't decide this anymore. For me it's just noise.

@asterite
Copy link
Member

Nevermind, let's fix to_s to use to_s. But please fix all of these:

  • Array
  • Hash
  • Tuple
  • Slice
  • StaticArray
  • Deque
  • Set

@RX14
Copy link
Contributor

RX14 commented Jan 23, 2018

I think that knowing the runtime type of something is a huge part of the inspect output. Inspect is largely used for debugging and knowing the type when debugging is hugely useful for reasoning about its behaviour.

@bcardiff
Copy link
Member

@bew would you want to include the rest of the containers in this PR?

@bcardiff
Copy link
Member

bcardiff commented Jan 23, 2018

The failing spec shows a case that we are not analyzing.
["foo"].to_s should be ["foo"] (T.inspect) or [foo] (T.to_s).

Maybe it's wrong to assume that there is a human (non-programmer) readable form of the containers, hence the current behaviour could be ok.

@bew
Copy link
Contributor Author

bew commented Jan 23, 2018

@bcardiff yes, I can do that later today

@asterite
Copy link
Member

Yup, that's a good reason why Array uses inspect. Plus, Array is really for programmers, not a general output that you should be sending down the wire.

I think there's nothing to do here: don't use Array#to_s nor Array#inspect if you don't want to get type info.

@RX14 RX14 closed this Jan 23, 2018
@bew
Copy link
Contributor Author

bew commented Jan 24, 2018

Ok, just understood your comment @bcardiff, that's a good point. Let's leave it that way then.

@bew bew deleted the Array#to_s-uses-T#to_s branch February 3, 2018 00:36
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.

None yet

5 participants