Add page to list all crate's items #49504
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Your PR failed on Travis. Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. Click to expand the log.If this was not helpful or you have suggestes for improvements, please ping or otherwise contact |
GuillaumeGomez
force-pushed
the
doc-all-types
branch
from
601f85b to
024fd46
Compare
|
Your PR failed on Travis. Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
shepmaster
added
the
S-waiting-on-author
|
|
GuillaumeGomez
force-pushed
the
doc-all-types
branch
from
024fd46 to
500d3f5
Compare
|
|
|
Your PR failed on Travis. Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
GuillaumeGomez
force-pushed
the
doc-all-types
branch
from
500d3f5 to
4a9444a
Compare
There was a problem hiding this comment.
Please add a src/test/rustdoc test to ensure that all.html gets links from a crate and any re-exported items as well.
Speaking of re-exports, what happens when an item is re-exported within the same crate, and both paths are public (think a prelude)? Do both paths appear in the listing? Reading over the source, it seems like if an item isn't inlined at its re-export path, it wouldn't generate a page, and thus wouldn't have a chance to insert itself into the AllTypes directory, but what about exports decorated with #[doc(inline)]? Should we care that both paths get links?
| functions: HashSet<ItemEntry>, | ||
| typedefs: HashSet<ItemEntry>, | ||
| statics: HashSet<ItemEntry>, | ||
| constants: HashSet<ItemEntry>, |
There was a problem hiding this comment.
Note for the future: these could all get replaced with FxHashSets later on, if this becomes a performance problem.
| functions: HashSet::with_capacity(100), | ||
| typedefs: HashSet::with_capacity(100), | ||
| statics: HashSet::with_capacity(100), | ||
| constants: HashSet::with_capacity(100), |
There was a problem hiding this comment.
Initializing everything here with 100 items (except the primitives, since there's not an unbounded number of those) seems like way overkill, but i don't know how the average crate measures up here. Is there a way to measure crates.io for this? >_>
There was a problem hiding this comment.
I just picked a random number. But at least with this, it's done.
src/librustdoc/html/render.rs
Outdated
| "fn" => self.functions.insert(ItemEntry::new(new_url, name)), | ||
| "typedef" => self.typedefs.insert(ItemEntry::new(new_url, name)), | ||
| "static" => self.statics.insert(ItemEntry::new(new_url, name)), | ||
| "constant" => self.constants.insert(ItemEntry::new(new_url, name)), |
There was a problem hiding this comment.
Would this be worth turning into an enum?
There was a problem hiding this comment.
Not so sure it'd be useful. It's the only place where it's used. Maybe if we extend it in the future it might be worth it but for now, I think the current solution seems the best one.
There was a problem hiding this comment.
I looked at how this was populated, and it looks like this is sourced from html::item_type::ItemType anyway. It's not using all the variants of it, but it's at least better than matching on strings, IMO.
src/librustdoc/html/render.rs
Outdated
| @@ -1414,16 +1544,52 @@ impl Context { | |||
| Some(i) => i, | |||
| None => return Ok(()), | |||
| }; | |||
| let final_file = self.dst.join(&krate.name) | |||
| .join(&format!("all{}.html", self.shared.resource_suffix)); | |||
There was a problem hiding this comment.
Does all.html really need the resource suffix? It doesn't like something that would benefit from it.
There was a problem hiding this comment.
It's for docs.rs. I suppose they'll have use for it.
There was a problem hiding this comment.
@onur Can you confirm whether this new page would require a resource suffix? It would be different per-crate, a new page linked from the crate-root sidebar.
There was a problem hiding this comment.
@GuillaumeGomez only common javascript and css files requires suffix, crate specific html files don't need one.
There was a problem hiding this comment.
Ok, I'll remove it then.
| @@ -389,3 +389,8 @@ kbd { | |||
| background: #f0f0f0; | |||
| } | |||
| } | |||
|
|
|||
| #all-types {} | |||
There was a problem hiding this comment.
This ID gets a background-color in the light theme; is it valid to leave it blank here? That seems like it will cause some problems at some point. >_>
There was a problem hiding this comment.
It doesn't need to overload the rule at the opposite of light.css so I left it empty. But I can update it, as you wish.
There was a problem hiding this comment.
I'm wary of relying on the rules combining properly if we ever add more themes. I'd rather see each theme define its own color set, even if some colors are the same as some other themes.
GuillaumeGomez
force-pushed
the
doc-all-types
branch
from
acaeca1 to
0534b1a
Compare
|
Added test. |
GuillaumeGomez
force-pushed
the
doc-all-types
branch
from
0534b1a to
43815a5
Compare
|
Updated. |
src/librustdoc/html/render.rs
Outdated
| </div> | ||
| <a id='all-types' href='all{}.html'><p>See all {}'s items</p></a>", | ||
| version, | ||
| cx.shared.resource_suffix, |
There was a problem hiding this comment.
This doesn't need the resource-suffix any more either.
src/librustdoc/html/render.rs
Outdated
| "fn" => self.functions.insert(ItemEntry::new(new_url, name)), | ||
| "typedef" => self.typedefs.insert(ItemEntry::new(new_url, name)), | ||
| "static" => self.statics.insert(ItemEntry::new(new_url, name)), | ||
| "constant" => self.constants.insert(ItemEntry::new(new_url, name)), |
There was a problem hiding this comment.
I looked at how this was populated, and it looks like this is sourced from html::item_type::ItemType anyway. It's not using all the variants of it, but it's at least better than matching on strings, IMO.
src/librustdoc/html/render.rs
Outdated
| } | ||
|
|
||
| let mut w = BufWriter::new(File::create(&final_file) | ||
| .expect("failed to create all.html")); |
There was a problem hiding this comment.
Don't we usually use try_err! to wrap errors from file operations? This function even returns a Result already.
src/librustdoc/html/render.rs
Outdated
| layout::render(&mut w, &self.shared.layout, | ||
| &page, &sidebar, &all, | ||
| self.shared.css_file_extension.is_some(), | ||
| &self.shared.themes).expect("layout rendering failed"); |
There was a problem hiding this comment.
Similar try_err! comment here as well, this returns an io::Result that we can handle with that macro.
|
Updated. |
|
@bors r+ Thanks so much! I'm really glad to see this happen. |
|
📌 Commit 1292e51 has been approved by |
bors
added
S-waiting-on-bors
bors
added a commit
that referenced
this pull request
Apr 10, 2018
Add page to list all crate's items r? @QuietMisdreavus
|
☀️ Test successful - status-appveyor, status-travis |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
r? @QuietMisdreavus