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

Remove integer suffixes from Show implementation #20792

Closed
wants to merge 1 commit into from

Conversation

tbu-
Copy link
Contributor

@tbu- tbu- commented Jan 9, 2015

They just clutter the output, reading

[0i32, 1i32, 2i32, 3i32]

is really hard.

They just clutter the output, reading
```
[0i32, 1i32, 2i32, 3i32]
```
is really hard.
@rust-highfive
Copy link
Collaborator

r? @brson

(rust_highfive has picked a reviewer for you, use r? to override)

@huonw
Copy link
Member

huonw commented Jan 9, 2015

These suffixes were part of the design fwiw. (I'm not expressing an opinion on whether they should be removed.)

@tbu-
Copy link
Contributor Author

tbu- commented Jan 9, 2015

It's what's written in the design, but it didn't have much discussion. We also don't distinguish between printing Vec, &[], or String, &str, this is just for debugging views of underlying values.

This also negatively affects tests: tbu-/collect-rs@18069a4#diff-8a5ac7b178f2c29d23992135dce5db83R969

@aturon
Copy link
Member

aturon commented Jan 9, 2015

FWIW, I'm hoping to post an RFC shortly establishing clearer conventions for fmt::Show and fmt::String. This may be best discussed there.

@tbu-
Copy link
Contributor Author

tbu- commented Jan 9, 2015

It's a pretty clear win in readability, so this could be merged without an RFC (?). Still need to fix stds tests though.

@alexcrichton
Copy link
Member

cc rust-lang/rfcs#565, we may wish to hold off on a decision here until a decision is reached on that RFC

@aturon
Copy link
Member

aturon commented Jan 9, 2015

FWIW, the RFC suggests making the change here. I do agree we should wait until that discussion is finished, though, to avoid unnecessary churn.

@alexcrichton
Copy link
Member

Thanks again for the PR @tbu-! The relevant RFC has now been accepted, so this is good to go. I ended up implementing this as well in #21457 (sorry I forgot about this PR!), so I'm going to close this in favor of that to reduce merge conflicts.

bors added a commit that referenced this pull request Jan 24, 2015
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.

6 participants