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

Remove unused imports from docs tests #4628

Merged
merged 6 commits into from
Feb 27, 2024
Merged

Conversation

Harsh1s
Copy link
Contributor

@Harsh1s Harsh1s commented Feb 23, 2024

Cleaned all the unused imports, which can be confusing to clients and developers.

Part of #4570

@CLAassistant
Copy link

CLAassistant commented Feb 23, 2024

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@robertbastian robertbastian left a comment

Choose a reason for hiding this comment

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

Thanks! Please also update the job in tests.toml with the arguments you used so that we don't backslide.

@sffc
Copy link
Member

sffc commented Feb 23, 2024

Thanks for doing this! Two things:

  1. Please check the test-docs CI and make sure all the docs tests are still passing.
  2. As @robertbastian suggested, please modify the job to run with the correct flags to prevent backsliding. You can set the flags in [tasks.test-docs] in tools/make/tests.toml here:
    [tasks.test-docs]

Copy link
Member

@sffc sffc left a comment

Choose a reason for hiding this comment

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

^

@Harsh1s
Copy link
Contributor Author

Harsh1s commented Feb 24, 2024

I have fixed the failing test now, it happened cuz I removed a legit import accidently.

Now about the test.toml arguements, from my research, I found out that rustdoc does not currently warn about unused imports. It’s a known limitation of rustdoc. So since there are no warnings generated, it is kind of difficult to prevent backsliding as you guys adviced. I'm still trying a few things but just wanted to keep you in the loop.

@Harsh1s
Copy link
Contributor Author

Harsh1s commented Feb 25, 2024

Here's an update of what I found:
"You can show warnings in doctests by running rustdoc --test --test-args=--show-output" is what the rust documentation says. So I went ahead and ran RUSTFLAGS="-D warnings" RUSTDOCFLAGS="--test-args --show-output" cargo test --all-features --doc --no-fail-fast on the command line. Unfortunately, there was not a single warning concerning unused imports. This makes it kind of complicated as no warnings mean nothing to abort or fix automatically.

@robertbastian I hope I'm not doing something absolutely stupid, would like to hear your advice on this.

robertbastian
robertbastian previously approved these changes Feb 26, 2024
Copy link
Member

@robertbastian robertbastian left a comment

Choose a reason for hiding this comment

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

I've looked into this and couldn't find a way to do this either. There's an open issue for this (rust-lang/rust#56232), but no workaround afaict. Let's merge the manual fixes for now.

Edit: please fix broken tests

@Harsh1s
Copy link
Contributor Author

Harsh1s commented Feb 26, 2024

I've looked into this and couldn't find a way to do this either. There's an open issue for this (rust-lang/rust#56232), but no workaround afaict. Let's merge the manual fixes for now.

Edit: please fix broken tests

Yeah unfortunately it was a wild goose chase :(
On the bright side though, manually removing them helped me understand the project more. Hope to make good contributions moving forward!

P.S. I've fixed the broken tests, @robertbastian thanks for your help with the issue!

@robertbastian robertbastian merged commit 17f2cb0 into unicode-org:main Feb 27, 2024
30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants