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

refactor range examples #35759

Closed

Conversation

matthew-piziak
Copy link
Contributor

This pull request adds a module-level example of how all the range operators work. It also slims down struct-level examples in lieu of a link to module examples.

@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 @alexcrichton (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. Due to 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 the contribution instructions for more information.

@matthew-piziak
Copy link
Contributor Author

r? @steveklabnik

//! assert_eq!(arr[1..3], [ 1,2 ]); // Range
//!
//! assert_eq!(arr[ ...3], [0,1,2,3 ]); // RangeToIncusive
//! assert_eq!(arr[1...3], [ 1,2,3 ]); // RangeInclusive
Copy link
Member

Choose a reason for hiding this comment

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

I know you copied these from the existing examples, but I wonder if we should eliminate the extra spacing, since it's not idiomatic Rust. Hmm

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could just add a note to that effect.

On Aug 17, 2016 2:59 PM, "Steve Klabnik" notifications@github.com wrote:

In src/libcore/ops.rs
#35759 (comment):

@@ -64,6 +64,20 @@
//!
//! See the documentation for each trait for a minimum implementation that
//! prints something to the screen.
+//!
+//! This example shows the behavior of the various Range* structs.
+//!
+//! ```rust
+//! let arr = [0, 1, 2, 3, 4];
+//!
+//! assert_eq!(arr[ .. ], [0,1,2,3,4]); // RangeFull
+//! assert_eq!(arr[ ..3], [0,1,2 ]); // RangeTo
+//! assert_eq!(arr[1.. ], [ 1,2,3,4]); // RangeFrom
+//! assert_eq!(arr[1..3], [ 1,2 ]); // Range
+//!
+//! assert_eq!(arr[ ...3], [0,1,2,3 ]); // RangeToIncusive
+//! assert_eq!(arr[1...3], [ 1,2,3 ]); // RangeInclusive

I know you copied these from the existing examples, but I wonder if we
should eliminate the extra spacing, since it's not idiomatic Rust. Hmm


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/rust-lang/rust/pull/35759/files/7564da22d890784af83bcdebac404c2d6d674bf7#r75185100,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AGfGaWOEno8eN_VLR0fTdvOGkxKdZBxTks5qg1oVgaJpZM4JmxaP
.

Copy link
Contributor

Choose a reason for hiding this comment

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

This documents unstable syntax, so if we want to do that it needs #![feature(inclusive_range_syntax)].

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added, thank you.

@matthew-piziak
Copy link
Contributor Author

^ done. Thanks!

@steveklabnik
Copy link
Member

@bors: r+ rollup

@bors
Copy link
Contributor

bors commented Aug 19, 2016

📌 Commit 9542d11 has been approved by steveklabnik

bors added a commit that referenced this pull request Aug 20, 2016
@bors
Copy link
Contributor

bors commented Aug 20, 2016

☔ The latest upstream changes (presumably #35857) made this pull request unmergeable. Please resolve the merge conflicts.

@matthew-piziak matthew-piziak force-pushed the refactor-range-examples branch 2 times, most recently from 25886a0 to 31b7ff4 Compare August 22, 2016 21:03
@steveklabnik
Copy link
Member

@bors: r+ rollup

@bors
Copy link
Contributor

bors commented Aug 25, 2016

📌 Commit 711333f has been approved by steveklabnik

steveklabnik added a commit to steveklabnik/rust that referenced this pull request Aug 25, 2016
…les, r=steveklabnik

refactor range examples

This pull request adds a module-level example of how all the range operators work. It also slims down struct-level examples in lieu of a link to module examples.
steveklabnik added a commit to steveklabnik/rust that referenced this pull request Aug 26, 2016
…les, r=steveklabnik

refactor range examples

This pull request adds a module-level example of how all the range operators work. It also slims down struct-level examples in lieu of a link to module examples.
steveklabnik added a commit to steveklabnik/rust that referenced this pull request Aug 26, 2016
…les, r=steveklabnik

refactor range examples

This pull request adds a module-level example of how all the range operators work. It also slims down struct-level examples in lieu of a link to module examples.
bors added a commit that referenced this pull request Aug 30, 2016
@TimNN
Copy link
Contributor

TimNN commented Aug 30, 2016

This PR fails linkchecker (see rollup #36126):

core/ops/enum.RangeInclusive.html:84: directory link - core
core/ops/struct.Range.html:76: directory link - core
core/ops/struct.RangeFrom.html:80: directory link - core
core/ops/struct.RangeFull.html:85: directory link - core
core/ops/struct.RangeTo.html:74: directory link - core
core/ops/struct.RangeToInclusive.html:78: directory link - core
std/ops/enum.RangeInclusive.html:84: directory link - std
std/ops/struct.Range.html:76: directory link - std
std/ops/struct.RangeFrom.html:80: directory link - std
std/ops/struct.RangeFull.html:85: directory link - std
std/ops/struct.RangeTo.html:74: directory link - std
std/ops/struct.RangeToInclusive.html:78: directory link - std

To quote a comment from linkchecker:

Links to directories show as directory listings when viewing the docs offline so it's best to avoid them.

@GuillaumeGomez
Copy link
Member

@bors: r-

bors added a commit that referenced this pull request Aug 30, 2016
@bors
Copy link
Contributor

bors commented Aug 30, 2016

☔ The latest upstream changes (presumably #36126) made this pull request unmergeable. Please resolve the merge conflicts.

This pull request adds a module-level example of how all the range operators work. It also slims down struct-level examples in lieu of a link to module examples.

add feature for inclusive_range_syntax

fix incorrectly closed code fences
@steveklabnik
Copy link
Member

Travis looks spurious, let's give it a try with bors.

@bors: r+ rollup

@bors
Copy link
Contributor

bors commented Sep 1, 2016

📌 Commit 34be21d has been approved by steveklabnik

sophiajt pushed a commit to sophiajt/rust that referenced this pull request Sep 2, 2016
…les, r=steveklabnik

refactor range examples

This pull request adds a module-level example of how all the range operators work. It also slims down struct-level examples in lieu of a link to module examples.
@frewsxcv
Copy link
Member

frewsxcv commented Sep 2, 2016

Fails tests:

failures:
---- ops::RangeToInclusive_2 stdout ----
    error: inclusive range syntax is experimental (see issue #28237)
 --> <anon>:5:17
  |
5 | assert_eq!(arr[ ...2], [0, 1, 2]);
  |                 ^^^^
  |
  = help: add #![feature(inclusive_range_syntax)] to the crate attributes to enable
error: aborting due to previous error(s)
thread 'ops::RangeToInclusive_2' panicked at 'Box<Any>', src/librustc/session/mod.rs:170
note: Run with `RUST_BACKTRACE=1` for a backtrace.
failures:
    ops::RangeToInclusive_2
test result: FAILED. 1051 passed; 1 failed; 12 ignored; 0 measured
error: test failed

@sophiajt
Copy link
Contributor

sophiajt commented Sep 2, 2016

@bors r-

Pulling it out for #35759 (comment) (and so I can re-roll the rollup). Once fixed we can re-approve and put it back into the queue.

@@ -2192,11 +2219,13 @@ impl<Idx: PartialOrd<Idx>> RangeInclusive<Idx> {
/// array elements up to and including the index indicated by `end`.
///
/// ```
/// #![feature(inclusive_range_syntax)]
Copy link
Contributor

Choose a reason for hiding this comment

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

This line shouldn't have been removed.

@TimNN
Copy link
Contributor

TimNN commented Sep 3, 2016

I'm pretty sure that this will fail tidy since directory links are used (ie. ../#Examples instead of ../index.html#Examples. (Also, the correct anchor is #examples, at least in chrome the matching seems to be case sensitive).

@matthew-piziak
Copy link
Contributor Author

Alas, local make tidy is quite noisy for me.

@TimNN
Copy link
Contributor

TimNN commented Sep 3, 2016

Sorry, it's actually linkchecker (and not tidy) that complains (which only runs when using rustbuild). You can however see the current failures on travis (at the very end) (I would paste them here, put apparently iOS/Safari is not capable of copying from large plaintext files).

@frewsxcv
Copy link
Member

frewsxcv commented Sep 3, 2016

Linkcheck stage2 (x86_64-unknown-linux-gnu)
core/ops/struct.RangeToInclusive.html:95: directory link - core
core/ops/struct.RangeFull.html:85: directory link - core
core/ops/struct.RangeTo.html:90: directory link - core
core/ops/struct.Range.html:76: directory link - core
core/ops/enum.RangeInclusive.html:84: directory link - core
core/ops/struct.RangeFrom.html:80: directory link - core
std/ops/struct.RangeToInclusive.html:95: directory link - std
std/ops/struct.RangeFull.html:85: directory link - std
std/ops/struct.RangeTo.html:90: directory link - std
std/ops/struct.Range.html:76: directory link - std
std/ops/enum.RangeInclusive.html:84: directory link - std
std/ops/struct.RangeFrom.html:80: directory link - std
thread 'main' panicked at 'found some broken links', src/tools/linkchecker/main.rs:54
note: Run with `RUST_BACKTRACE=1` for a backtrace.


command did not execute successfully: "/build/build/x86_64-unknown-linux-gnu/stage2-tools/x86_64-unknown-linux-gnu/release/linkchecker" "/build/build/x86_64-unknown-linux-gnu/doc"
expected success, got: exit code: 101

@frewsxcv
Copy link
Member

Linkcheck stage2 (x86_64-unknown-linux-gnu)
core/ops/struct.RangeToInclusive.html:95: broken link fragment  `#examples` pointing to `core/index.html`
core/ops/struct.RangeFull.html:85: broken link fragment  `#examples` pointing to `core/index.html`
core/ops/struct.RangeTo.html:90: broken link fragment  `#examples` pointing to `core/index.html`
core/ops/struct.Range.html:76: broken link fragment  `#examples` pointing to `core/index.html`
core/ops/enum.RangeInclusive.html:84: broken link fragment  `#examples` pointing to `core/index.html`
core/ops/struct.RangeFrom.html:80: broken link fragment  `#examples` pointing to `core/index.html`
std/ops/struct.RangeToInclusive.html:95: broken link fragment  `#examples` pointing to `std/index.html`
std/ops/struct.RangeFull.html:85: broken link fragment  `#examples` pointing to `std/index.html`
std/ops/struct.RangeTo.html:90: broken link fragment  `#examples` pointing to `std/index.html`
std/ops/struct.Range.html:76: broken link fragment  `#examples` pointing to `std/index.html`
std/ops/enum.RangeInclusive.html:84: broken link fragment  `#examples` pointing to `std/index.html`
std/ops/struct.RangeFrom.html:80: broken link fragment  `#examples` pointing to `std/index.html`
thread 'main' panicked at 'found some broken links', src/tools/linkchecker/main.rs:54
note: Run with `RUST_BACKTRACE=1` for a backtrace.

@steveklabnik
Copy link
Member

@matthew-piziak ping! any interest in keeping up with this PR?

@bors
Copy link
Contributor

bors commented Sep 30, 2016

☔ The latest upstream changes (presumably #36864) made this pull request unmergeable. Please resolve the merge conflicts.

@matthew-piziak
Copy link
Contributor Author

@steveklabnik Sorry, I've been traveling for the past three weeks. I should be back in the game around 7 October. Right now this PR is soft-blocking on #36417. Feel free to close; I'll keep the branch alive on my fork.

@steveklabnik
Copy link
Member

@matthew-piziak no problem, since it's blocked on something else, and you're willing to keep the branch going, let's close it for now so that I don't have to remember that every time I look through the queue. Safe travels!

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.