NEEDLESS_OPTION_TAKE doc improvements

[alex-semenyuk]

changelog: More info on NEEDLESS_OPTION_TAKE

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Jarcho (or someone else) soon.

Please see the contribution instructions for more information.

[Jarcho]

Can you expand on why calling take is redundant rather than just stating that it is.

[alex-semenyuk]

Can you expand on why calling take is redundant rather than just stating that it is.

@Jarcho added more details.

[Jarcho]

Usually the Why is this bad? section is just one or two sentences unless there are subtleties that require more explanation. A detailed step-by-step breakdown isn't usually needed. I should have been more clear about the level of detail needed here.

Your explanation is also missing why x.as_ref().take() should be linted, but x.take() is fine. You covered that take returns the input value, but not why take does nothing after as_ref.

[alex-semenyuk]

@Jarcho Thanks for review!
My intention was to show that the result x.as_ref() and x.as_ref().take() are the same, that's why take() after x.as_ref() is needless. As for size it's actually few sentences if remove example.

[Jarcho]

Between the example code and the type listing of every expression there's a lot of extra detail in the explanation. Only two points are required for understanding why .as_ref().take() is redundant.

• take is modifying the value returned by as_ref, which is a new, temporary value.
• take returns the value passed in as it's argument.

Anything not related to those two points ends up as a distraction from them.

As an example, the last line along with steps three and four are the only parts needed to point out they return the same value. The rest of it ends up getting in the way of seeing what the main point is.

[flip1995]

This section should conceptually explain why this is bad, rather than with examples. If the examples are not good enough, please add better examples in the Examples section. But I would prefer to keep this section free of code examples for all lints (there are always exceptions of course).

[alex-semenyuk]

@flip1995 updated

[alex-semenyuk]

@Jarcho updated

[Jarcho]

That is an improvement with the extra focus. This can be further simplified to just the last sentence. It's enough to just state that the results are the same without walking through the types involved.

You need to add something about take writing None to it's argument. In this case the modification is useless as it's to a temporary that cannot be read from afterwards.

[alex-semenyuk]

@Jarcho ok

[Jarcho]

Looks good. Thank you for catching the missing docs.

@bors r+

[bors]

📌 Commit e780435 has been approved by Jarcho

It is now in the queue for this repository.

[bors]

⌛ Testing commit e780435 with merge 3a4e457...

[bors]

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: Jarcho
Pushing 3a4e457 to master...