-
Notifications
You must be signed in to change notification settings - Fork 258
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
Must_use messages #995
Comments
I suspect the right answer is either to just ditch the message altogether (and only use it for actual novel meaningful situations), or alternatively link to a page that describes how we use the builder-lite pattern in Ratatui and why |
Another point which is kinda annoying here: There is both a doc comment and a /// This is a fluent setter method which must be chained or used as it consumes self
#[must_use = "method moves the value of self and returns the modified value"] It should have either of those, and I would prefer a |
Once ratatui-org/ratatui-website#558 finishes deploying, perhaps we can change these all to just: #[must_use("https://ratatui.rs/concepts/builder-lite-pattern")] |
It should keep a simple message in there. Just pointing to a URL is not very friendly. "The return value contains your object. Read more about it on x" seems more friendly. |
I think predominantly (at least from what I've seen) the std lib doesn't add any message for must_use at all. Here's the compiler warning without any added message:
Here's the same with just the bare url:
Perhaps this is enough:
I think the message we're trying to convey is that this isn't a mutable setter method, it's a method which returns the value with the updated field set. What about: "This is a builder lite method. See https://ratatui.rs/concepts/builder-lite-pattern"? I suspect that we're over-indexing on a problem that even the newest rustacean should be able to get past (the curse of knowledge unfortunately means that it's difficult to know if this suspicion is correct, but a useful proxy for working that out is the click stats on that page) In times past, I've had good success and feedback from users just putting a url to every possible error (and it makes it easy to update the explanation without re-releasing the software). So it might be even better to just link e.g. https://ratatui.rs/warnings/must-use instead. |
With the example output: yeah. Nothing or an URL seems like a good enough thing. It’s clear enough that the result is relevant and needs to be used. |
Cool. My preferences in order would be:
I don't have a strong preference for any of the first three over the other, so a bare /concepts url works if that's what you prefer. |
A warning having a note: output including warning… not sure if I like that. I understand the reasoning. It explains the warning further. But I think a concept is more relevant there. That's why its this way. But all together… A person will either just use the compiler output to improve that and does not care about the pattern ratatui was built with, or that person reads into the ratatui then a concept is more helpful. I think nothing would also be perfectly fine and the easiest to maintain / not to mess up on additions. So I'm for nothing or a concept URL. |
Let's go with the concepts url. :) |
For example for Widgets could be " |
This not really accurate any more. |
In some places in can have weird side effects, in others that's really neat. See #1130 for some experiments with them. |
Not something entirely specific to this PR as it’s in other places too…
The ownership of the original object is not moved so the message is confusing with ownership in mind.
It actually does not change the value of self and returns a new, modified value. With a lifetime reference shared with the original.
Not sure how often these must_use messages are misleading in other places too… maybe we should create an issue about it to check that?
Originally posted by @EdJoPaTo in #987 (comment)
The text was updated successfully, but these errors were encountered: