Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(style): sync submodule exclusion list between tidy and rustfmt #132524

Merged
merged 1 commit into from
Nov 6, 2024
Merged

chore(style): sync submodule exclusion list between tidy and rustfmt #132524

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions rustfmt.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,6 @@ ignore = [
"/tests/ui-fulldeps/", # Some are whitespace-sensitive (e.g. `// ~ERROR` comments).

# Do not format submodules.
# FIXME: sync submodule list with tidy/bootstrap/etc
# tidy/src/walk.rs:filter_dirs
"library/backtrace",
"library/portable-simd",
"library/stdarch",
Expand All @@ -41,6 +39,7 @@ ignore = [
"src/llvm-project",
"src/tools/cargo",
"src/tools/clippy",
"src/tools/enzyme",
"src/tools/miri",
"src/tools/rust-analyzer",
"src/tools/rustc-perf",
Expand Down
1 change: 0 additions & 1 deletion src/tools/tidy/src/walk.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ use ignore::DirEntry;

/// The default directory filter.
pub fn filter_dirs(path: &Path) -> bool {
// FIXME: sync submodule exclusion list with rustfmt.toml
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussion: @onur-ozkan should tidy be reading from rustfmt.toml or we somehow make sure that there's a single source of truth for this? (not for this PR)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Skip lists for rustfmt and tidy seem not overlapping but if tidy needs to include the skip list of rustfmt, I wanna work on it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

My other concern is that highlighted rows in the screenshot above (from rustfmt.toml) are not submodules. Maybe some time ago, they were and added to that list. But now they are managed inside the rust repo and rustfmt doesn't format them. I've tried to run x fmt for them and found a lot of errors. Shouldn't they be formatted like other files in the rust repo as well?

Copy link
Member

@jieyouxu jieyouxu Nov 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some of them are not submodules but subtrees and managed by josh, so I don't think this is easily sync'd. Asking Onur because I might be missing context for these particular lists.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC we don't want tidy to rely on a toml parser because it needs to build fast, so probably not sync'd easily.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So how about relying on .gitmodules file for both rustfmt and tidy?

// bootstrap/etc
let skip = [
"tidy-test-file",
Expand Down
Loading