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

Better print #496

Merged
merged 1 commit into from
Mar 22, 2015
Merged

Better print #496

merged 1 commit into from
Mar 22, 2015

Conversation

mdinger
Copy link
Contributor

@mdinger mdinger commented Mar 8, 2015

Aims at fixing #495 by adding more detail about printing. Don't merge.

I gotta go do other things now but this is what I've got so far. May not get back to it for a few days.

First commit was a driveby fix.

@rust-highfive
Copy link

r? @steveklabnik

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

@mdinger
Copy link
Contributor Author

mdinger commented Mar 8, 2015

cc @crumblingstatue You may be interested in this.

@strega-nil
Copy link

Wow, this is awesome. 👍


// Error! You are missing an argument.
println!("My name is {0:?}, {1:?} {0:?}", "Bond");
// FIXME ^ Add the missing argument: "James"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you should remove the FIXME here. It's helpful to tell the user exactly what he needs to do to fix this problem.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I botched the build somewhere and I think I deleted this while tracking down the error. Forgot to add it back.

@mdinger
Copy link
Contributor Author

mdinger commented Mar 13, 2015

Pushed updates. Was working on this offline so I'm not sure how it gels with the previous comments. I'll have to compare again later.

I tried adding print! and format! but they're so similar that it didn't seem worth the extra space. In general, long examples aren't good so I dropped them. I've still gotta look it over again though.

@steveklabnik
Copy link
Member

Cool, when would you like me to review this?

@mdinger
Copy link
Contributor Author

mdinger commented Mar 13, 2015

I wanna sleep on it and look at it again tomorrow. I'll try to ping you tomorrow or the next day for review. I think it'll be ready then. I don't think there will be many big changes though. I'll update title from WIP too (gotta remember to do it...)

@mdinger
Copy link
Contributor Author

mdinger commented Mar 15, 2015

Something came up. Haven't gotten to it yet. I'll try to get to it in a day or so.

@mdinger mdinger changed the title WIP: Better print Better print Mar 16, 2015
@mdinger
Copy link
Contributor Author

mdinger commented Mar 16, 2015

@steveklabnik r?
I rebased onto that other pull so I could build it. I don't know what it'll do if you push the other first. I assume it'll work though. I don't have any particularly strong criticism remaining about it other than Display is kinda long and I don't see much sense in splitting it into more sections.

@crumblingstatue I think I moved that specific struct you commented about and I tried to improve the naming in general though they follow naming schemes that makes sense to me. In general, I try to keep struct definitions outside of main() and rustbyexample historically does that too. I don't have any issues moving them inside where it makes sense though.

As before, comments welcome.

@steveklabnik
Copy link
Member

I rebased onto that other pull so I could build it. I don't know what it'll do if you push the other first. I assume it'll work though

Oh, I didnt see this till now...

@mdinger
Copy link
Contributor Author

mdinger commented Mar 16, 2015

You want me to fix it? I'm kinda curious what it'll do...

@steveklabnik
Copy link
Member

It looks like everything is fine, right?

@mdinger
Copy link
Contributor Author

mdinger commented Mar 16, 2015

I think so. Since the first 3 patches already merged, it should just ignore them and add only the last. That's speculation though. If you don't want to risk it, I can rebase again so there's only a single patch.

Also, rather than closing #495 if you merge, can you check all but the last box. Or leave it to me. I'll do it.

@mdinger
Copy link
Contributor Author

mdinger commented Mar 19, 2015

@steveklabnik I went ahead and rebased to tip because it makes the PR cleaner though the commit/merge history shouldn't be different either way.

steveklabnik added a commit that referenced this pull request Mar 22, 2015
@steveklabnik steveklabnik merged commit e497040 into rust-lang:master Mar 22, 2015
@steveklabnik
Copy link
Member

Okay! Let's land this. Thanks.

@mdinger
Copy link
Contributor Author

mdinger commented Mar 22, 2015

Cool

@mdinger mdinger deleted the better_print branch March 22, 2015 02:15
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.

5 participants