Doc alias improvements #71724
Merged
Doc alias improvements #71724
Conversation
This file contains 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
|
The job 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-alias-improvements
branch
from
61be7ed
to
70fcb74
Compare
ollie27
suggested changes
May 2, 2020
There was a problem hiding this comment.
There are a few things we can do to simplify this. How about adding an aliases field to IndexItem to store the list of aliases for each item. Then the actual aliases map can be built inside build_index after the paths and parents for each item have been fully calculated. Additionally we can add the aliases map to CrateData so it will be included in search-index.js allowing us to remove the aliases field on Cache and the seperate aliases.js file.
GuillaumeGomez
force-pushed
the
doc-alias-improvements
branch
from
82d2bb7
to
a293680
Compare
|
@ollie27 I don't know why I didn't think about putting aliases in the search index before, this is a brilliant idea! However, I don't like the idea to put the aliases into Anyway, I integrated your suggestions. Thanks a lot! |
ollie27
suggested changes
May 6, 2020
GuillaumeGomez
force-pushed
the
doc-alias-improvements
branch
2 times, most recently
from
4081845
to
b0c5e2c
Compare
|
Updated! |
|
The job 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-alias-improvements
branch
from
b0c5e2c
to
2fded7c
Compare
|
Fixed tidy issue... |
|
The job 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-alias-improvements
branch
from
2fded7c
to
1b84e80
Compare
|
And with this one I added the missing comma in the JS test (long day). |
ollie27
suggested changes
May 11, 2020
src/librustdoc/html/render/cache.rs
Outdated
| @@ -582,6 +553,12 @@ fn build_index(krate: &clean::Crate, cache: &mut Cache) -> String { | |||
| parent_idx: None, | |||
| search_type: get_index_search_type(&item), | |||
| }); | |||
| for alias in item.attrs.get_doc_aliases().into_iter() { | |||
There was a problem hiding this comment.
Suggested change
| for alias in item.attrs.get_doc_aliases().into_iter() { | |
| for alias in item.attrs.get_doc_aliases() { |
When I make a suggestion like this it applies to all instances not just the one I comment on. This is why we should be trying to avoid code duplication.
|
|
||
| for alias in item.attrs.get_doc_aliases().into_iter() { | ||
| self.aliases | ||
| .entry(alias.to_lowercase()) |
There was a problem hiding this comment.
I guess it's not a big issue for now. It does seem weird that aliases are treated differently to the other search results though.
|
Updated, the code is indeed much better! |
ollie27
approved these changes
May 13, 2020
GuillaumeGomez
force-pushed
the
doc-alias-improvements
branch
from
9050eed
to
e553226
Compare
…terministic results * Update Javascript to take this change into account * Update CrateData::aliases field to take a reference instead (it allowed to remove a conversion loop)
GuillaumeGomez
force-pushed
the
doc-alias-improvements
branch
from
e553226
to
e17ac66
Compare
|
I squashed a few commits but kept the big changes into their own commits if this is ok with you. |
|
cc @ollie27 |
There are still commits that makes changes that subsequent commits just revert. Feel free to r=me if you think it's too much work to clean up the commits or you don't want to squash into one commit. It's the end result that matters most after all. |
|
Well, for me it's important to keep one change per commit. It makes the git history easier to follow. Even more considering how much the tests impact here. I squashed commits in order to keep it clean so I'll r+ it like this. Thanks a lot for your great reviews! Once this PR is merged, I'll need to check what remains to be done so this feature can finally be stabilized. @bors: r+ |
|
📌 Commit e17ac66 has been approved by |
bors
added
the
S-waiting-on-bors
|
Erf... @bors: r- |
bors
added
S-waiting-on-author
|
@bors: r=ollie27 |
|
📌 Commit e17ac66 has been approved by |
bors
added
S-waiting-on-bors
Dylan-DPC-zz
pushed a commit
to Dylan-DPC-zz/rust
that referenced
this pull request
May 15, 2020
…ts, r=ollie27 Doc alias improvements After [this message](rust-lang#50146 (comment)), I realized that the **doc alias**. So this PR does the followings: * Align the alias discovery on items added into the search-index. It brings a few nice advantages: * Instead of cloning the data between the two (in rustdoc source code), we now have the search-index one and aliases which reference to the first one. So we go from one big map containing a lot of duplicated data to just integers... * In the front-end (main.js), I improved the code around aliases to allow them to go through the same transformation as other items when we show the search results. * Improve the search tester in order to perform multiple requests into one file (I think it's better in this case than having a file for each case considering how many there are...) * I also had to add the new function inside the tester (`handleAliases`) Once this PR is merged, I intend to finally stabilize this feature. r? @ollie27 cc @rust-lang/rustdoc
Dylan-DPC-zz
pushed a commit
to Dylan-DPC-zz/rust
that referenced
this pull request
May 15, 2020
…ts, r=ollie27 Doc alias improvements After [this message](rust-lang#50146 (comment)), I realized that the **doc alias**. So this PR does the followings: * Align the alias discovery on items added into the search-index. It brings a few nice advantages: * Instead of cloning the data between the two (in rustdoc source code), we now have the search-index one and aliases which reference to the first one. So we go from one big map containing a lot of duplicated data to just integers... * In the front-end (main.js), I improved the code around aliases to allow them to go through the same transformation as other items when we show the search results. * Improve the search tester in order to perform multiple requests into one file (I think it's better in this case than having a file for each case considering how many there are...) * I also had to add the new function inside the tester (`handleAliases`) Once this PR is merged, I intend to finally stabilize this feature. r? @ollie27 cc @rust-lang/rustdoc
Dylan-DPC-zz
pushed a commit
to Dylan-DPC-zz/rust
that referenced
this pull request
May 15, 2020
…ts, r=ollie27 Doc alias improvements After [this message](rust-lang#50146 (comment)), I realized that the **doc alias**. So this PR does the followings: * Align the alias discovery on items added into the search-index. It brings a few nice advantages: * Instead of cloning the data between the two (in rustdoc source code), we now have the search-index one and aliases which reference to the first one. So we go from one big map containing a lot of duplicated data to just integers... * In the front-end (main.js), I improved the code around aliases to allow them to go through the same transformation as other items when we show the search results. * Improve the search tester in order to perform multiple requests into one file (I think it's better in this case than having a file for each case considering how many there are...) * I also had to add the new function inside the tester (`handleAliases`) Once this PR is merged, I intend to finally stabilize this feature. r? @ollie27 cc @rust-lang/rustdoc
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
May 16, 2020
Rollup of 9 pull requests Successful merges: - rust-lang#71662 (Implement FromStr for OsString) - rust-lang#71677 (Add explicit references to the BuildHasher trait) - rust-lang#71724 (Doc alias improvements) - rust-lang#71948 (Suggest to await future before ? operator) - rust-lang#72090 (rustc_driver: factor out computing the exit code) - rust-lang#72206 (Cleanup stale 'FIXME(rust-lang#64197)') - rust-lang#72218 (make sure even unleashed miri does not do pointer stuff) - rust-lang#72220 ([const-prop] Don't replace Rvalues that are already constants) - rust-lang#72224 (doc: add links to rotate_(left|right)) Failed merges: r? @ghost
I agree but that's not what we have. Take for example cf41b1d which adds |
|
Oh, I completely overlooked it, sorry about this too... |
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.
After this message, I realized that the doc alias. So this PR does the followings:
handleAliases)Once this PR is merged, I intend to finally stabilize this feature.
r? @ollie27
cc @rust-lang/rustdoc