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

Updated documentation to use range notation syntax #21834

Merged
merged 3 commits into from
Feb 14, 2015
Merged

Updated documentation to use range notation syntax #21834

merged 3 commits into from
Feb 14, 2015

Conversation

genbattle
Copy link
Contributor

Replaced outdated use of the range(start, end) function where appropriate with start..end, and tweaked the examples to compile and run with the latest rust. I also fixed two periphery compile issues in reference.md which were occluding whether there were any new errors created by these changes.

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @steveklabnik (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. The way Github handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see CONTRIBUTING.md for more information.

@@ -2994,7 +2994,7 @@ Some examples of call expressions:
# fn add(x: i32, y: i32) -> i32 { 0 }

let x: i32 = add(1i32, 2i32);
let pi: Option<f32> = "3.14".parse().ok();
let pi: Option<f32> = "3.14".parse();
Copy link
Contributor

Choose a reason for hiding this comment

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

let pi: Result<f32, _> = "3.14".parse(); The API has been changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When was it updated? The documentation still seems to refer to it returning an Option.

Copy link
Contributor

Choose a reason for hiding this comment

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

It is #21718, just yesterday fresh out of the oven.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have no compiler to test against, the latest rust-nightly-bin package is (1d00c54 2015-01-30 19:56:34 +0000). I can't fix this issue and verify that it works until the nightly is updated to include your change; currently I just get:

[nick@hacksaw range-test]$ rustc main.rs 
main.rs:12:30: 12:44 error: mismatched types:
 expected `core::result::Result<f32, _>`,
    found `core::option::Option<_>`
(expected enum `core::result::Result`,
    found enum `core::option::Option`) [E0308]
main.rs:12     let pi: Result<f32, _> = "3.14".parse();
                                        ^~~~~~~~~~~~~~

I have no time to work on this tonight or tomorrow night, but I may be able to compile the latest rustc on Wednesday night local (GMT+12).

Copy link
Member

Choose a reason for hiding this comment

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

You have Option< in that error message, not Result<.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why not open a separate PR to update all the doc examples affected by #21718 instead of cramming it into this one? src/doc/trpl/guessing-game.md has at least 9 occurrences of this error.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's, of course, a viable solution too. Fine with me.

@steveklabnik
Copy link
Member

This needs a rebase now, but I'm interested in moving it forward.

@genbattle
Copy link
Contributor Author

I know how to rebase my branch against master in my fork, but I haven't used github enough to figure out the best way to update my fork with mainline rust - github seems to suggest pulling the changes from mainline rust via a pull request into my fork. Is this the best way to go about it, or is there a cleaner/easier way to do this?

@genbattle
Copy link
Contributor Author

No matter, figured it out, rebasing now.

@steveklabnik
Copy link
Member

I have this setup:

$ git remote -v
origin  git@github.com:steveklabnik/rust.git (fetch)
origin  git@github.com:steveklabnik/rust.git (push)
upstream        https://github.com/rust-lang/rust.git (fetch)
upstream        https://github.com/rust-lang/rust.git (push)

and then

$ git checkout mybranch
$ git fetch upstream
$ git rebase upstream/master # with all that this entails...
$ git push origin -f mybranch

@genbattle
Copy link
Contributor Author

After I did the rebase git insisted I pull and merge with my remote, but obviously in this case a push -f would have been the better option. It seems to have worked, although I don't know what it's done to the history :-/

@fujin
Copy link

fujin commented Feb 6, 2015

Use the reflog to hard move your branch pointer back before you pull/merged, rebase again, check fast-forward, then git push --force-with-lease. 0

@genbattle
Copy link
Contributor Author

I reset to before the pull/merge and force pushed, it seems to have corrected it?

@fujin
Copy link

fujin commented Feb 6, 2015

@genbattle Indeed; Thank you.

@genbattle
Copy link
Contributor Author

Steve, don't push this through yet. I just realized I don't think I ended
up fixing up newly created occurrences of range(start, end) when I did the
merge for the rebase, I'll get onto this in the morning and add another
commit to this PR.
On 6 Feb 2015 6:49 pm, "AJ Christensen" notifications@github.com wrote:

@genbattle https://github.com/genbattle Indeed; Thank you.


Reply to this email directly or view it on GitHub
#21834 (comment).

@genbattle
Copy link
Contributor Author

There was only one additional instance of the range function instead of range notation, so I've fixed it up, tested it, and added it to this branch.

@steveklabnik
Copy link
Member

This looks great, @genbattle , except the original comment about Result. Do these tests pass? Other than that, this looks great, r=me

Replaced outdated use of the `range(start, end)` function where
approriate with `start..end`, and tweaked the examples to compile and run with the latest rust. I also fixed two periphery compile issues in reference.md which were occluding whether there were any new errors created by these changes, so I fixed them.
@genbattle
Copy link
Contributor Author

@steveklabnik I've fixed the usage of StrExt.parse() that @edwardw originally commented on. I also fixed all similar occurrences in doc/trpl/guessing-game.md.

I did another rebase and tested against all of the examples in all of the files touched by this PR and fixed any errors that arose. It should be all good to go unless you have any more comments or recommendations for me?

@steveklabnik
Copy link
Member

@bors: r+ 8300095

@steveklabnik
Copy link
Member

Nope, looks great! Thnak you! (not rolling up since this is a big ish change)

@bors
Copy link
Contributor

bors commented Feb 13, 2015

⌛ Testing commit 8300095 with merge 4760750...

@bors
Copy link
Contributor

bors commented Feb 13, 2015

💔 Test failed - auto-win-64-opt

@steveklabnik
Copy link
Member

@bors: retry

@bors
Copy link
Contributor

bors commented Feb 13, 2015

⌛ Testing commit 8300095 with merge 5b543c1...

@bors
Copy link
Contributor

bors commented Feb 13, 2015

💔 Test failed - auto-mac-64-opt

@alexcrichton
Copy link
Member

@bors: retry

@bors
Copy link
Contributor

bors commented Feb 14, 2015

⌛ Testing commit 8300095 with merge f174bca...

bors added a commit that referenced this pull request Feb 14, 2015
Replaced outdated use of the `range(start, end)` function where appropriate with `start..end`, and tweaked the examples to compile and run with the latest rust. I also fixed two periphery compile issues in reference.md which were occluding whether there were any new errors created by these changes.
@bors bors merged commit 8300095 into rust-lang:master Feb 14, 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.

7 participants