adds new trait-method-added lint #891
Conversation
src/lints/trait_method_added.ron
Outdated
| item { | ||
| ... on Trait { | ||
| visibility_limit @filter(op: "=", value: ["$public"]) @output | ||
| sealed @filter(op: "!=", value: ["$true"]) |
There was a problem hiding this comment.
This one's tricky, and could have gone either way. So far, we've been treating "the trait got sealed" and "new items requiring implementation got added" as separate issues which should both lint. That's how the other lints in this category work too.
For consistency, we should remove this.
| sealed @filter(op: "!=", value: ["$true"]) |
Would you mind adding a test case that covers this as well?
| /* | ||
| Will let this case to be reported only by the newly sealed trait Lint, | ||
| and not by this one, since sealing a trait indicates that the user | ||
| wants to remove this type from the public API. | ||
| */ |
There was a problem hiding this comment.
This is correct, and like I said earlier, it's a judgment call that could go either way.
The opposite argument is that the user might not have realized that their trait is sealed (it has become so by accident). In that case, since it wasn't an intentional decision, items requiring implementation are a separate and unrelated issue.
| method { | ||
| method_name: name @output @tag | ||
| has_body @filter(op: "!=", value: ["$true"]) | ||
|
|
||
| span_: span @optional { | ||
| filename @output | ||
| begin_line @output | ||
| } | ||
| } |
There was a problem hiding this comment.
This portion is correctly implemented, but would be lovely if we had a test case for one more situation that users might find confusing.
Users might think that marking the new method #[doc(hidden)] may resolve this lint. It shouldn't, and it'd be great to have a test for it.
For bonus points, if you're interested, you could take a look at our other "trait item added without default" lints and make sure they all have a "#[doc(hidden)] on the new item" test case as well. If not, more PRs are welcome!
… adds tests for it
|
If you're open to adding tests for |
see #870