[WIP] rust case insensitive fs support #59785
Closed
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
GuillaumeGomez
added
the
T-rustdoc
rust-highfive
added
the
S-waiting-on-review
|
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 |
|
At some point this will be changed to be enabled by default for Windows users? |
|
I hope so? I'd prefer something to detect the filesystem. But for now, there is still some work to be done in here. |
QuietMisdreavus
suggested changes
Apr 11, 2019
| &mut self, | ||
| krate: &clean::Crate, | ||
| ) -> Result<(), Error> { | ||
| /*if !self.shared.case_insensitive { |
There was a problem hiding this comment.
It looks like the check that would make it bail (the only thing that reads the new case_insensitive parameter) is still commented out?
There was a problem hiding this comment.
The point was to test it when generating std without having to change boostrap, I'll remove it once this PR is done.
| /// Returns `true` if the `collapse-docs` pass was run on this crate. | ||
| pub fn was_collapsed(&self) -> bool { | ||
| self.passes.contains("collapse-docs") | ||
| } | ||
|
|
||
| /// Based on whether the `collapse-docs` pass was run, return either the `doc_value` or the | ||
| /// `collapsed_doc_value` of the given item. | ||
| pub fn maybe_collapsed_doc_value<'a>(&self, item: &'a clean::Item) -> Option<Cow<'a, str>> { | ||
| pub fn maybe_collapsed_doc_value<'b>(&self, item: &'b clean::Item) -> Option<Cow<'b, str>> { |
There was a problem hiding this comment.
Why did you change the name of the lifetime parameter here?
There was a problem hiding this comment.
At some point I added a parameter with a lifetime to SharedContext, making necessary to make this change. It's completely useless now.
| @@ -703,8 +731,34 @@ pub fn run(mut krate: clean::Crate, | |||
| cx.krate(krate) | |||
| } | |||
|
|
|||
| fn url_to_path(url: &str, start_path: &str) -> Option<String> { | |||
There was a problem hiding this comment.
Can you add a doc comment describing what this function is supposed to do? It took me too long to try to figure out what's happening here.
| @@ -1254,8 +1316,11 @@ themePicker.onblur = handleThemeButtonsBlur; | |||
| Ok(()) | |||
| } | |||
|
|
|||
| fn render_sources(dst: &Path, scx: &mut SharedContext, | |||
| krate: clean::Crate) -> Result<clean::Crate, Error> { | |||
| fn render_sources<'a>( | |||
There was a problem hiding this comment.
It looks like this lifetime parameter was unnecessary?
| /// all sub-items which need to be rendered. | ||
| /// | ||
| /// The rendering driver uses this closure to queue up more work. | ||
| fn item<F>(&mut self, item: clean::Item, all: &mut AllTypes, mut f: F, |
There was a problem hiding this comment.
Why did you move this method? It makes it hard to read the PR, since git won't show me if you needed to change it.
There was a problem hiding this comment.
Yes sorry, I was just going down and back between two functions and this one was in the middle so I moved it. Just like the rest, I'll put it "back" once done.
| @@ -2452,14 +2605,15 @@ impl<'a> fmt::Display for Item<'a> { | |||
| } | |||
| } | |||
| write!(fmt, "<a class=\"{}\" href=''>{}</a>", | |||
| self.item.type_(), self.item.name.as_ref().unwrap())?; | |||
| self.item.type_(), self.item.name.as_ref().expect("name is unavailable 4"))?; | |||
There was a problem hiding this comment.
Why is all this unrelated code being changed? It makes it hard to read the PR because of all this extra noise between the substantial additions. If you really want to add the unwrap/expect conversions to this PR, can you at least break it into its own commit?
There was a problem hiding this comment.
Simply because I had a failure and I couldn't find from where so to track it down, I replaced most unwrap calls with expect.
| @@ -3101,7 +3277,7 @@ fn item_trait( | |||
| VisSpace(&it.visibility), | |||
| UnsafetySpace(t.unsafety), | |||
| if t.is_auto { "auto " } else { "" }, | |||
| it.name.as_ref().unwrap(), | |||
| it.name.as_ref().expect("name unavailable 10"), | |||
There was a problem hiding this comment.
There's two "name unavailable 10" messages. (And i'm not a fan of having the separate numbered messages just to make them different - we can get the line number in the backtrace.)
| containing_item: &clean::Item, | ||
| it: DefId, | ||
| what: AssocItemRender<'_>) -> fmt::Result { | ||
| fn render_assoc_items( |
There was a problem hiding this comment.
Did you run rustfmt on this, or did you manually change all these signatures? Please avoid including unrelated code style changes in the same commit as substantial changes - it inflates the line count and creates misleading blame messages.
There was a problem hiding this comment.
The function changed a few times following my needs and this is the result, nothing magical behind that. ;)
|
cc #25879 |
|
@QuietMisdreavus: But more globally, do you think this is the correct way to do this? Or should I try to put it into the |
|
ping from triage @QuietMisdreavus @GuillaumeGomez any updates on this? |
|
Still waiting for @QuietMisdreavus feedback. :) |
|
Does this also work for the redirect files, since those are grouped by namespace instead of just kind? pub struct Foo;
pub enum FoO {}@GuillaumeGomez The basic approach - generating a mapping of item paths to filename stems in case of conflict - is fine. However, my complaints about the PR being difficult to review still stand. The changes are sprawling, divided into commits that cannot be viewed on their own, offer no explanation of the overall approach or specific implementation details, and grouped in with unrelated (and in many cases unnecessary) changes. I can't offer comments on your approach if it takes all my energy to just pick apart what your PR is doing in the first place. |
|
Ok, I'll try to clean it up but considering the amount of changes, I think it'll be complicated in any case. I'll add comments though and write down a global explanation of what I have in mind. |
|
☔ The latest upstream changes (presumably #62041) made this pull request unmergeable. Please resolve the merge conflicts. |
|
I'm going to close this PR for the time being, it can be reopened once @GuillaumeGomez finishes rebasing and reworking the PR into a more reviewable state. |
|
Hey, would it be kosher for me to try to make the necessary changes to make it reviewable and open a new pull request with said changes? |
|
I think so, yes, though I myself am currently uncertain as to the approach in general :) |
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.
This PR aims the support for case insensitive file systems in such cases:
I now need people to take a look and look for bugs that I'd have missed (windows' users would be very cool!).
I plan on adding the code sample above as a test but that seems a bit limited compared to the amount of code there is...
r? @QuietMisdreavus