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.

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

Add recursive config setting to appropriate tests #4113

Closed
calebcartwright opened this issue Apr 10, 2020 · 6 comments
Closed

Add recursive config setting to appropriate tests #4113

calebcartwright opened this issue Apr 10, 2020 · 6 comments
Labels
good first issue Issues up for grabs, also good candidates for new rustfmt contributors hacktoberfest help wanted

Comments

@calebcartwright
Copy link
Member

calebcartwright commented Apr 10, 2020

rustfmt 1.x ran in "recursive" mode by default wherein all the mods within the AST would be visited and formatted, including those mods defined in external files not explicitly passed as args to rustfmt.

rustfmt 2.x inverts that behavior and will now only format files explicitly passed to rustfmt unless the --recursive flag is passed (in which case rustfmt 2.x formats everything like rustfmt 1.x)

As such a few of our test cases needed to be updated to explicitly include the recursive. Currently these tests are running in "non-recursive" mode and unintentionally ignoring some files which has resulted in a few false positves.

This can be addressed by adding the below snippet to the top of the corresponding pairs of test files under rustfmt-core/rustfmt-lib/tests/{source, target}/**

// rustfmt-recursive: true

Some (perhaps all) of the tests that need this change can be found by reviewing the skipped files in rustfmt-core/rustfmt-lib/src/test/mod.rs to determine the corresponding entry point files that need recursive enabled.

const SKIP_FILE_WHITE_LIST: &[&str] = &[
"issue-3434/no_entry.rs",
"issue-3665/sub_mod.rs",
// Testing for issue-3779
"issue-3779/ice.rs",
// These files and directory are a part of modules defined inside `cfg_if!`.
"cfg_if/mod.rs",
"cfg_if/detect",
"issue-3253/foo.rs",
"issue-3253/bar.rs",
"issue-3253/paths",
// These files and directory are a part of modules defined inside `cfg_attr(..)`.
"cfg_mod/dir",
"cfg_mod/bar.rs",
"cfg_mod/foo.rs",
"cfg_mod/wasm32.rs",
// We want to ensure `recursive` is working correctly, so do not test
// these files directly
"configs/recursive/disabled/foo.rs",
"configs/recursive/enabled/foo.rs",
];

For example, we can see there's some test files within the cfg_if directory (rustfmt-core/rustfmt-lib/tests/{source,target}/cfg_if/mod.rs that are being explicitly ignored to validate the target behavior, so the entry test file in this case (rustfmt-core/rustfmt-lib/tests/{source,target}/cfg_if/lib.rs) needs to have recursive mode included by adding // rustfmt-recursive: true

This would be a good first issue for anyone interested in working on rustfmt!

@calebcartwright calebcartwright added good first issue Issues up for grabs, also good candidates for new rustfmt contributors help wanted labels Apr 14, 2020
@Krout0n
Copy link

Krout0n commented Apr 30, 2020

Hi, can I tackle this for my first time contributing rustfmt?

@calebcartwright
Copy link
Member Author

calebcartwright commented May 1, 2020

Hi @Krout0n 👋 Thanks for your interest in contributing to rustfmt!

@bradleypmartin is working on this one already (see #4142), but there are plenty of other issues in the backlog if you'd be interested in working on a different one,

You may want to check out the issues that have been tagged with good-first-issue and/or those with help-wanted to see if you'd like to try any of those (though don't feel constrained to just those issues).

If you find one that you want to work on just drop a note on that issue, and you can ask us any questions on that thread and/or in the wg-rustfmt channel on the Rust Discord server

@kellda
Copy link

kellda commented Jul 25, 2020

Seems this issues should have been closed by #4142

@calebcartwright
Copy link
Member Author

Thanks @kellda, but #4142 explicitly didn't cover all the tests that needed to be update which is why this was deliberately left open. It's possible the other tests may have been subsequently addressed via other PRs but going to keep this open until that's definitively determined.

@sasurau4
Copy link
Contributor

sasurau4 commented Dec 13, 2020

The list to add recursive config is following.
I'd happy adding these checks in the issue description.

@sasurau4
Copy link
Contributor

@calebcartwright This issue seems completely done. 🎉
Could this issue be close?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Issues up for grabs, also good candidates for new rustfmt contributors hacktoberfest help wanted
Projects
None yet
Development

No branches or pull requests

4 participants