Add support for windows-style paths in ignore patterns #3633
Conversation
src/ignore_path.rs
Outdated
| ignore_builder.add_line(None, ignore_path.to_str().unwrap())?; | ||
| let ignore_line = ignore_path.to_str().unwrap(); | ||
| if cfg!(windows) { | ||
| ignore_builder.add_line(None, &ignore_line.replace("\\", "/"))?; |
There was a problem hiding this comment.
Seems like one downside of only doing this on Windows is that projects utilizing ignore with these windows style paths could end up with different experiences with rustfmt on different platforms.
A cargo fmt -- --check could pass for a developer/CI engine on Windows but fail for a developer/CI engine on Mac, Linux, etc.
There was a problem hiding this comment.
You are right, this is a real concern. However, it seems that the #[path = "..."] attribute does not support Windows-style paths in other environments as well:
// error: couldn't read foo\bar.rs: No such file or directory (os error 2)
#[path = "foo\\bar.rs"]
mod foobar;I will just take this behavior an excuse for rustfmt that it's ok not to support the Windows-style path outside of Windows 😉
While you are this, could you add some documentation in Configurations.md about this, please?
There was a problem hiding this comment.
I will just take this behavior an excuse for rustfmt that it's ok not to support the Windows-style path outside of Windows 😉
In that case, do you think that Windows-style paths should not be supported in rustfmt's ignore setting as well? Using regular / style paths in rustfmt ignore config works fine for me on both my Ubuntu and Windows machines, so I'd personally prefer to just use the / style that works on all platforms.
Instead of making this code change, could we just update the Configurations information to instruct users to use / paths for ignore setting?
There was a problem hiding this comment.
Now that you mention it, I think that sounds more reasonable.
Instead of making this code change, could we just update the Configurations information to instruct users to use / paths for ignore setting?
Would you mind updating this PR with this direction? Sorry for multiple requests for changes, this should be the last one.
|
Thank you! |
Fixes #3537
I also checked on my windows machine and confirmed this works