-
Notifications
You must be signed in to change notification settings - Fork 543
docs: document new suggest-tests
tool
#1660
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
Conversation
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.
Overall great, a few typos and a bigger question
src/tests/suggest-tests.md
Outdated
the resulting command or does "one size fit all"? | ||
2. Based on the previous step, decide if your suggestion should be implemented | ||
as either static or dynamic. | ||
3. Implement the suggestion. If it is dynamic then a test is required, see the |
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.
Is the fact that the tests are required enforced somewhere (Lint/check?) Not a blocker, but would be good as a follow-up. In the meantime, say something like
3. Implement the suggestion. If it is dynamic then a test is required, see the | |
3. Implement the suggestion. If it is dynamic then a test is highly recommended, to verify that your logic is correct and to give an example of the suggestion. See the |
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.
Good idea. I'll add it to the tracking issue. Not entirely sure how it could be implmented though.
42c92f9
to
7a8cade
Compare
@albertlarsan68 Implemented. I was far too tired when I wrote and proofread this :P. |
@rustbot label -blocked @albertlarsan68 This can now be merged, the main PR has been merged. |
as either static or dynamic. | ||
3. Implement the suggestion. If it is dynamic then a test is highly recommended, | ||
to verify that your logic is correct and to give an example of the suggestion. | ||
See the [tests.rs](https://github.com/rust-lang/rust/blob/master/src/tools/suggest-tests/src/static_suggestions.rs) |
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.
See the [tests.rs](https://github.com/rust-lang/rust/blob/master/src/tools/suggest-tests/src/static_suggestions.rs) | |
See the [tests.rs](https://github.com/rust-lang/rust/blob/master/src/tools/suggest-tests/src/tests.rs) |
Update books ## rust-lang/book 1 commits in c06006157b14b3d47b5c716fc392b77f3b2e21ce..8fa6b854d515506d825390fe0d817f5ef0c89350 2023-04-13 00:05:30 UTC to 2023-04-13 00:05:30 UTC - Update copyright in LICENSE-APACHE (rust-lang/book#3611) ## rust-embedded/book 1 commits in 701d1551429da4cb609082c0ac99df569e336710..897fcf566f16bf87bf37199bdddec1801fd00532 2023-04-20 13:24:51 UTC to 2023-04-20 13:24:51 UTC - typos (rust-embedded/book#347) ## rust-lang/rustc-dev-guide 5 commits in 6337ed1..2a5eb92 2023-04-22 11:50:11 UTC to 2023-04-16 11:30:24 UTC - Add docs for compare-output-lines-by-subset flag (rust-lang/rustc-dev-guide#1677) - fix typo (rust-lang/rustc-dev-guide#1674) - Fix links in how-to-build-and-run.md (rust-lang/rustc-dev-guide#1679) - docs: document new `suggest-tests` tool (rust-lang/rustc-dev-guide#1660) - Fix extra slash (rust-lang/rustc-dev-guide#1673)
Part of rust-lang/rust#109933.
Blocked on rust-lang/rust#106249? (the links are broken until it's merged)
I think it's a pretty good overview, both high and low level.
r? @albertlarsan68