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

Should format_collect be warn by default? #11434

Open
smoelius opened this issue Aug 30, 2023 · 3 comments · May be fixed by #13894
Open

Should format_collect be warn by default? #11434

smoelius opened this issue Aug 30, 2023 · 3 comments · May be fixed by #13894

Comments

@smoelius
Copy link
Contributor

smoelius commented Aug 30, 2023

Description

format_collect suggests to replace format! with write!.

A downside of this suggestion is that it replaces a infallible function with a fallible one (i.e., write! returns a Result, whereas format! does not).

format_push_string has a similar downside, and it was moved to restriction for this reason.

Should the same standard apply to format_collect? Or is something about its situation different?

cc: @y21, @Centri3

@y21
Copy link
Member

y21 commented Aug 30, 2023

Yeah, we were discussing the category a bit on the PR and centri3 suggested pedantic I think. I wasn't aware of that lint. I do kinda agree now that warn by default might be a bit too strong and maybe this should be in restriction, seeing that format_push_string is also and the arguments given.
It's a bit unfortunate that write! on a string, an operation that can't fail, returns a Result, thus requiring an unnecessary let _ = or .unwrap(), but it makes sense given that the macro goes through the generic std::fmt::Write machinery.

@smoelius
Copy link
Contributor Author

smoelius commented Aug 30, 2023

Rereading my post, though, I didn't mean to suggest that format_collect should be restriction.

I too think pedantic could make sense.

(EDIT: And similarly for format_push_string.)

@y21
Copy link
Member

y21 commented Sep 10, 2023

Right, so I think format_collect and format_push_string should probably be in the same category, since they're basically checking for the same kind of pattern but in different places. So there's two options, then:

  1. Move format_collect to restriction and leave format_push_string where it is
  2. Move format_collect and format_push_string to pedantic

I personally think 2) is better (I haven't found a strong argument as to why it should be restriction specifically), but I created a poll on zulip for this:
https://rust-lang.zulipchat.com/#narrow/stream/257328-clippy/topic/.60format_collect.60.20category

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 a pull request may close this issue.

2 participants