Skip to content

Advertise Vec in LinkedList docs#38297

Merged
bors merged 1 commit into
rust-lang:masterfrom
matklad:linked-lists-are-not-cool
Dec 25, 2016
Merged

Advertise Vec in LinkedList docs#38297
bors merged 1 commit into
rust-lang:masterfrom
matklad:linked-lists-are-not-cool

Conversation

@matklad

@matklad matklad commented Dec 11, 2016

Copy link
Copy Markdown
Contributor

r? @steveklabnik

Hi! We already advise to use Vec instead of LinkedList in the top-level collections documentation. But I think it may be missed by someone who just directly finds LinkedList.

What do you feel about advertising Vec directly in LinkedList docs as well?

@matklad

matklad commented Dec 11, 2016

Copy link
Copy Markdown
Contributor Author

Question: how should I cross-link the docs? Relative paths won't work for LinkedList structure, because it is visible as both collections::LinkedList and collections::linked_list::LinkedList.

@steveklabnik

Copy link
Copy Markdown
Contributor

Question: how should I cross-link the docs?

This is one area where we can't 😦

@ollie27

ollie27 commented Dec 13, 2016

Copy link
Copy Markdown
Contributor

You've hit #32130. The options are to leave the links out for now or add more exceptions to linkchecker here.

Comment thread src/libcollections/linked_list.rs Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Since this is a module-level doc comment, I guess LinkedList should have an url.

Comment thread src/libcollections/linked_list.rs Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We don't use this linking syntax, we use:

text long text etc [link] blabla
blablabla

[link]: url

Comment thread src/libcollections/linked_list.rs Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Same comment.

Comment thread src/libcollections/linked_list.rs Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Same comment.

Comment thread src/libcollections/linked_list.rs Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Same comment.

@matklad matklad force-pushed the linked-lists-are-not-cool branch 2 times, most recently from 1065395 to 8ea3d32 Compare December 14, 2016 12:26
@matklad

matklad commented Dec 14, 2016

Copy link
Copy Markdown
Contributor Author

So let's use links in the module docs, because they always work, and let's avoid links on the struct directly, because it is reexported.

@GuillaumeGomez

Copy link
Copy Markdown
Member
std/collections/linked_list/index.html:57: broken link - std/collections/linked_list/struct.LinkedList
collections/linked_list/index.html:56: broken link - vec/struct.Vec.html
collections/linked_list/index.html:57: broken link - collections/linked_list/struct.LinkedList

@matklad matklad force-pushed the linked-lists-are-not-cool branch 2 times, most recently from d46374d to 1c868ee Compare December 16, 2016 07:06
@matklad matklad force-pushed the linked-lists-are-not-cool branch from 1c868ee to 71de148 Compare December 16, 2016 07:47
@matklad

matklad commented Dec 16, 2016

Copy link
Copy Markdown
Contributor Author

Linking to Vec is also impossible, because there are collections and std::collections. Should be good now?

/// The `LinkedList` allows pushing and popping elements at either end
/// in constant time.
///
/// Almost always it is better to use `Vec` or `VecDeque` instead of

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why not linking to VecDeque here? You did it just above.

@matklad matklad Dec 17, 2016

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

LinkedList struct is visible as both https://doc.rust-lang.org/std/collections/struct.LinkedList.html and https://doc.rust-lang.org/std/collections/linked_list/struct.LinkedList.html, so I can't craft a relative link that will be valid in both cases.

but for the linked_list module I can do this.

@matklad

matklad commented Dec 20, 2016

Copy link
Copy Markdown
Contributor Author

@GuillaumeGomez anything else to clean up here?

@GuillaumeGomez

Copy link
Copy Markdown
Member

Nothing, thanks!

@bors: r+ rollup

@bors

bors commented Dec 21, 2016

Copy link
Copy Markdown
Collaborator

📌 Commit 71de148 has been approved by GuillaumeGomez

@bors

bors commented Dec 22, 2016

Copy link
Copy Markdown
Collaborator

⌛ Testing commit 71de148 with merge d365d32...

@bors

bors commented Dec 22, 2016

Copy link
Copy Markdown
Collaborator

💔 Test failed - auto-mac-32-opt

@steveklabnik

Copy link
Copy Markdown
Contributor

@bors: retry

steveklabnik added a commit to steveklabnik/rust that referenced this pull request Dec 24, 2016
…r=GuillaumeGomez

Advertise Vec in LinkedList docs

r? @steveklabnik

Hi! We already [advise](https://doc.rust-lang.org/std/collections/#use-a-linkedlist-when) to use `Vec` instead of `LinkedList` in the top-level collections documentation. But I think it may be missed by someone who just directly finds `LinkedList`.

What do you feel about advertising `Vec` directly in `LinkedList` docs as well?
bors added a commit that referenced this pull request Dec 24, 2016
Rollup of 14 pull requests

- Successful merges: #37956, #38013, #38297, #38480, #38497, #38502, #38505, #38513, #38521, #38549, #38554, #38557, #38568, #38572
- Failed merges:
@bors bors merged commit 71de148 into rust-lang:master Dec 25, 2016
@matklad matklad deleted the linked-lists-are-not-cool branch July 9, 2019 12:34
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