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

Fix example in documentation #361

Merged
merged 1 commit into from
May 10, 2017
Merged

Fix example in documentation #361

merged 1 commit into from
May 10, 2017

Conversation

rap2hpoutre
Copy link
Contributor

see #359

I choose to write:

assert_eq!("2010".to_string(), caps["year"]);

But I could have choose:

assert_eq!("2010", &caps["year"]);

Not sure which one is the best (maybe none of them)

Copy link
Member

@BurntSushi BurntSushi left a comment

Choose a reason for hiding this comment

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

I think I actually prefer the &caps["year"] approach better, since it's more likely to match up with what you'll need in actual code. That is, caps["year"] has type str, which usually doesn't work.

Also, could you include the string Fixes #359 in the commit message and make sure the PR is squashed to one commit?

Thanks for doing this!

@rap2hpoutre
Copy link
Contributor Author

Thanks!! I changed the approach and added Fixes #359 in commit. For the squash, do you need me to do it? Or will you do it in merging? (I remember github has an option to "squash and merge", but I can (try to) do it myself)

@BurntSushi
Copy link
Member

@rap2hpoutre We unfortunately don't use the Github UI to merge PRs. Instead, we have a bot do it (which checks that the merge passes CI before pushing to master, which Github doesn't do).

To squash, just do git rebase -i master in a checkout of your PR branch (but make sure your master branch is up to date before doing so). Then type squash next to your Fixes #359 commit. Save and quit your editor. And then touch up the commit message when git opens your editor again. Finally, git push origin patch-1 --force.

@rap2hpoutre
Copy link
Contributor Author

Done! Thanks again!

@BurntSushi
Copy link
Member

Poifect! Thanks! @bors r+

@bors
Copy link
Contributor

bors commented May 10, 2017

📌 Commit b9ac569 has been approved by BurntSushi

@bors
Copy link
Contributor

bors commented May 10, 2017

⌛ Testing commit b9ac569 with merge e9f8318...

bors added a commit that referenced this pull request May 10, 2017
Fix example in documentation

see #359

I choose to write:

```rust
assert_eq!("2010".to_string(), caps["year"]);
```

But I could have choose:

```rust
assert_eq!("2010", &caps["year"]);
```

Not sure which one is the best (maybe none of them)
@bors
Copy link
Contributor

bors commented May 10, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: BurntSushi
Pushing e9f8318 to master...

@bors bors merged commit b9ac569 into rust-lang:master May 10, 2017
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.

3 participants