Repository navigation
Use indoc for multiline strings in tests - #7169
Conversation
rustbot
added
the
S-waiting-on-review
d936ddf to
b1518c9
Compare
jieyouxu
added
the
llm-assisted
There was a problem hiding this comment.
For the picked instances here:
- I do find some of these tactically selected instances to be more readable, when the test intention is moreso "hey something has changed, I don't really care exactly how"
- But, sometimes I find that the original string literal version is better (more directly obvious) when for the test intention you care about the exact presence/absence (and quantity) of newlines and other whitespaces.
TL;DR: overall I feel neutral about the changes.
Use indoc where it makes visible indentation and before / after comparisons easier to read. Use raw strings for embedded quotes. Not all places were modified. Kept compact strings and escapes where they were subjectively easier to visually inspect. Indoc is only added as a dev-dependency to avoid increasing the runtime dependencies. Assisted-by: Codex, with prior discussion with the maintainers via Zulip https://rust-lang.zulipchat.com/#narrow/channel/357797-t-rustfmt/topic/Characterization.20tests/with/630013200 Codex identified candidates and made the fixes. I directed the scope and reviewed readability tradeoffs rejecting several changes that didn't seem to be worth the churn.
b1518c9 to
ac4369b
Compare
|
I dropped all the review items that were mentioned - no strong concerns. Mostly they felt subjective and I was 60/40 or 51/49 on a bunch of them as being better. I also added a panicdoc! macro to indoc, and it’s published in 2.0.8, so I added the two diagnostic conversions in 5c23798d. I validated that both helpers still produce the same panic messages. |
| semver = "1.0.21" | ||
|
|
||
| [dev-dependencies] | ||
| indoc = "2.0.8" |
There was a problem hiding this comment.
@ytmimi a quick vibecheck on the extra dev-dependency: do you think the changes here pull its weight? I more-or-less neutral with a lean towards "seems nicer for these cases".
There was a problem hiding this comment.
Yes—I think the places where indoc helps read a bit nicer, and I have more tests planned that would benefit similarly. I wrote a small dedent! macro as an alternative in that upcoming change (respecting the one LLM change in flight request to post this and the followups that sit on top of it), but that leaves rustfmt maintainers maintaining code that duplicates an established, stable solution.
My calculus is that indoc is a small crate—about 17 KB and 405 lines of code—with broad adoption. The readability benefit seems worth the dependency, especially as a dev-dependency.
Personally, I’d also be comfortable with it as a regular dependency, though I understand why a foundational tool like rustfmt would be more conservative. It feels like functionality that could reasonably belong in the standard library.
So I see little downside to using it for tests, and a relatively small tradeoff even for production code.
There was a problem hiding this comment.
Hm yeah. Since this is dev-deps and test-only, I think it's fine introducing it.
Re. the calculus, indoc also has the other variants for UX that I don't think we want to be maintaining ourselves.
rustbot
added
release-notes
|
Thanks for the review :) |
3 participants
Use indoc where it makes visible indentation and before / after
comparisons easier to read. Use raw strings for embedded quotes. Not all
places were modified. Kept compact strings and escapes where they were
subjectively easier to visually inspect. Indoc is only added as a
dev-dependency to avoid increasing the runtime dependencies.
Assisted-by: Codex, with prior discussion with the maintainers via Zulip
https://rust-lang.zulipchat.com/#narrow/channel/357797-t-rustfmt/topic/Characterization.20tests/with/630013200
Codex identified candidates and made the fixes.
I directed the scope and reviewed readability tradeoffs rejecting several
changes that didn't seem to be worth the churn.
Fixes: #7166