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

UseDataClass does not report for classes that implement interfaces #2904

Closed
zsmoore opened this issue Jul 29, 2020 · 10 comments
Closed

UseDataClass does not report for classes that implement interfaces #2904

zsmoore opened this issue Jul 29, 2020 · 10 comments
Labels
Milestone

Comments

@zsmoore
Copy link
Contributor

zsmoore commented Jul 29, 2020

Expected Behavior

UseDataClass should report for classes that implement interfaces without methods

Observed Behavior

UseDataClass does not report for classes that implement interfaces

Steps to Reproduce

interface Entity

data class(i: Int): Entity

Does not report

Context

In our app we have a class called ViewData to represent simple data classes in Java before we started using Kotlin.
Shifting to Kotlin we would like to have these classes be data classes however detekt does not report on these since the classes implement an interface

As per https://kotlinlang.org/docs/reference/data-classes.html data classes are permitted to implement interfaces

@cortinico
Copy link
Member

cortinico commented Jul 29, 2020

Sound like a valid addition. Thank you very much for taking the time to work on it 🙏

UseDataClass should report for classes that implement interfaces without methods

Why only for interfaces without methods? In your PR seems like you're ignoring this specific condition. Was that intentional?

nit: your example should be fixed as the correct one should be:

interface Entity

class(i: Int): Entity

EDIT: typo

@zsmoore
Copy link
Contributor Author

zsmoore commented Jul 29, 2020

Ah yes thanks I accidentally added the data to the example.

For my PR, looking at the existing code it should handle not considering classes that have methods inside them so allowing interface implementers to hit the existing code seemed to do the trick for me which is why I did not add any code to check what the interface was defined to do.

@schalkms
Copy link
Member

I don't think the user should use a data class here. The rule should flag simple cases, where a data class is obvious to use, but not every corner case. The rule was designed with simplicity in mind.

@arturbosch
Copy link
Member

I don't think the user should use a data class here. The rule should flag simple cases, where a data class is obvious to use, but not every corner case. The rule was designed with simplicity in mind.

Maybe I was to fast merging the PR without reading the issue ... I try to be better!
I agree that we should not add too many exceptions as this rule should really just find simple cases like @schalkms mentioned.

Imo this change is still okay because the rule checks if any new functions appear in this class through interfaces we won't report it. I will add a check with delegating interfaces.

@zsmoore
Copy link
Contributor Author

zsmoore commented Jul 30, 2020

Ahh I missed considering delegation in this one that is my bad.
I agree we should not flag delegation however, I would argue that keeping the rule for simple interfaces is a big benefit and not an edge case.

@schalkms
Copy link
Member

Good points were brought up here. Thank you for the productive discussion here! I think this rule works smooth now. 🙂

@arturbosch arturbosch added this to the 1.10.1 milestone Aug 1, 2020
@YashNagayach95
Copy link

Is there a way to ssupress a rule when my class is implementing a specific interface?

@BraisGabin
Copy link
Member

No, there isn't. Could you open a new issue explaining your use case and why would that be valuable for you? We could create a suppressor to support something like that.

@YashNagayach95
Copy link

No, there isn't. Could you open a new issue explaining your use case and why would that be valuable for you? We could create a suppressor to support something like that.

But it worked! I don't know how. My use case was, I want to ignore those files from detekt that are implementing some X interface. So what I did is, I added @Supress("MyIssue") above my X interface and it is working for all the classes those were implementing that X interface.
No idea if this is right or wrong.

@BraisGabin
Copy link
Member

Oh, I didn't know that it could work like that... I'm not sure if that's a desired or undefined behavior but, if it works for you everything is fine :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants