Skip to content

Conversation

novalis
Copy link
Contributor

@novalis novalis commented Sep 3, 2013

No description provided.

Copy link
Member

Choose a reason for hiding this comment

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

This actually isn't entirely true. The # character also must be escaped (it's a back-reference to the currently-being-formatted argument in nested functions.

@alexcrichton
Copy link
Member

That looks good to me. Two requests:

  1. Could you squash these all into one commit?
  2. While you're at it if you feel motivated, 'x' probably doesn't need the ' characters around the literal because it should already be separated as a small code block. Just a preference though, I don't mind either way.

@novalis
Copy link
Contributor Author

novalis commented Sep 4, 2013

Done.

In general, when editing pull requests to rust, should I squash as I go? Or does that get annoying for commenters whose comments get hidden away? (I'm happy to work either way; I just want to make everyone else happy too!)

@alexcrichton
Copy link
Member

I don't think there's really a good convention on it one way or another. I've never been that impressed with github's code review tools. It's unfortunate that all history is lost in the github review if you rebase, but I generally think that it's fine to rebase.

Thankfully if you do comment on the commit itself github still marks the old comments with "show outdated diff", so at least there's a small consolation. If you comment on the overall pull request diff though I have absolutely no idea what github does to track those between revisions.

Anyway, thanks for putting up with me :)

bors added a commit that referenced this pull request Sep 4, 2013
@bors bors closed this Sep 4, 2013
flip1995 pushed a commit to flip1995/rust that referenced this pull request Jun 4, 2022
Rustup

r? `@ghost`

changelog: none
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.

4 participants