-
Notifications
You must be signed in to change notification settings - Fork 13.3k
Add CSS tidy check #74368
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Add CSS tidy check #74368
Conversation
Some changes occurred in HTML/CSS/JS. Some changes occurred in HTML/CSS themes. |
Those messages are getting better and better. XD |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
r=me with nit fixed, though I'd love to see this squashed into two commits: tidy changes and the effects of fixing those.
src/tools/tidy/src/style.rs
Outdated
@@ -161,6 +174,10 @@ pub fn check(path: &Path, bad: &mut bool) { | |||
// currently), just the long error code explanation ones. | |||
return; | |||
} | |||
if filename.ends_with(".css") && !is_in(file, "librustdoc") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be is_style_file
perhaps?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
src/tools/tidy/src/style.rs
Outdated
@@ -136,15 +137,28 @@ macro_rules! suppressible_tidy_err { | |||
}; | |||
} | |||
|
|||
pub fn is_in(full_path: &Path, folder_to_find: &str) -> bool { | |||
if let Some(parent) = full_path.parent() { | |||
if parent.file_name().map(|f| f.to_string_lossy() == folder_to_find).unwrap_or(false) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if parent.file_name().map(|f| f.to_string_lossy() == folder_to_find).unwrap_or(false) { | |
if parent.file_name().map_or_else(false, |f| f.to_string_lossy() == folder_to_find) { |
Do we need to_string_lossy
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need a conversion to string
so I took the code from below and didn't change it much.
src/tools/tidy/src/style.rs
Outdated
@@ -136,15 +137,28 @@ macro_rules! suppressible_tidy_err { | |||
}; | |||
} | |||
|
|||
pub fn is_in(full_path: &Path, folder_to_find: &str) -> bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we stop iterating on the rust project directory?
let mut full_path = full_path;
while let Some(parent) = full_path.parent() {
if let Some(f) = parent.file_name() {
if f == folder_to_find {
return true;
} else if f == RUST_DIR {
return false;
}
}
full_path = parent;
}
false
Just an example, it could be shorten.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is RUST_DIR
in this case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where rust repository is, I don't know what should it be, is there something like this? I think yours is simpler but it may hit something else created by the user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, it's very unlikely but I can add a check on the parent as well (it might actually be a good idea).
@Mark-Simulacrum I cherry-picked the first commit from another PR of mine, this is why there are two commits of tidy fixes. Also github seems kinda down because I force-pushed with only 3 commits a while ago and they still don't show up... |
e94d521
to
529a921
Compare
24f0c68
to
63ffa13
Compare
Updated! |
@bors r+ |
📌 Commit 63ffa13 has been approved by |
…rk-Simulacrum Add CSS tidy check r? @Mark-Simulacrum
…rk-Simulacrum Add CSS tidy check r? @Mark-Simulacrum
This failed its own check #74412 (comment) |
…rk-Simulacrum Add CSS tidy check r? @Mark-Simulacrum
☔ The latest upstream changes (presumably #74422) made this pull request unmergeable. Please resolve the merge conflicts. |
From doing rollups I'm pretty sure some of the PRs that just landed break this, mind rebasing and making sure the new changes pass tidy? @bors r- |
The ayu improvements I assume. |
63ffa13
to
72d039d
Compare
Rebased. |
72d039d
to
83ffd5c
Compare
@bors: r=Mark-Simulacrum |
📌 Commit 83ffd5c has been approved by |
…rk-Simulacrum Add CSS tidy check r? @Mark-Simulacrum
…arth Rollup of 18 pull requests Successful merges: - rust-lang#71670 (Enforce even more the code blocks attributes check through rustdoc) - rust-lang#73930 (Make some Option methods const) - rust-lang#74009 (Fix MinGW `run-make-fulldeps` tests) - rust-lang#74056 (Add Arguments::as_str().) - rust-lang#74169 (Stop processing unreachable blocks when solving dataflow) - rust-lang#74251 (Teach bootstrap about target files vs target triples) - rust-lang#74288 (Fix src/test/run-make/static-pie/test-aslr.rs) - rust-lang#74300 (Use intra-doc links in core::iter module) - rust-lang#74364 (add lazy normalization regression tests) - rust-lang#74368 (Add CSS tidy check) - rust-lang#74394 (Remove leftover from emscripten fastcomp support) - rust-lang#74411 (Don't assign `()` to `!` MIR locals) - rust-lang#74416 (Use an UTF-8 locale for the linker.) - rust-lang#74424 (Move hir::Place to librustc_middle/hir) - rust-lang#74428 (docs: better demonstrate that None values are skipped as many times a…) - rust-lang#74438 (warn about uninitialized multi-variant enums) - rust-lang#74440 (Fix Arc::as_ptr docs) - rust-lang#74452 (intra-doc links: resolve modules in the type namespace) Failed merges: r? @ghost
r? @Mark-Simulacrum