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

Change MatchingDeclarationName to handle utility files #1500

Closed
scottkennedy opened this issue Feb 20, 2019 · 13 comments
Closed

Change MatchingDeclarationName to handle utility files #1500

scottkennedy opened this issue Feb 20, 2019 · 13 comments

Comments

@scottkennedy
Copy link
Contributor

Context

This is based on the discussion at the bottom of #841

I have a utility file. It consists almost exclusively of top-level functions. However, one of these returns an instance of a data class that is also declared in this file. Because that is the only top-level class/object, Detekt says the file should be named after that data class, even though it's fairly insignificant.

As @arturbosch suggested, something like allowClassesWithModifiers = "data,enum" may be a reasonable configuration option to handle this.

@schalkms
Copy link
Member

I see.
Why don't you put the data class in a separate file? A utility file shouldn't contain that stuff if possible in my opinion. Therefore, I am not a big fan of this configuration option.

@arturbosch
Copy link
Member

I think it can make sense when the data class is exclusively used as a return type of the top level functions instead of a pair or something. Having this data class in the same file should be fine :).

@schalkms
Copy link
Member

make sense when the data class is exclusively used as a return type of the top level functions instead of a pair or something

Why would you want to return a pair? You can also access the data class if its located in another file.

@arturbosch
Copy link
Member

make sense when the data class is exclusively used as a return type of the top level functions instead of a pair or something

Why would you want to return a pair? You can also access the data class if its located in another file.

If its logically aligns with the file's responsibility, I would totally prefer having it in the same file than in an other file next to this one. However if the package of this file, represents a specific domain, I would also go for a new file with only this data class. Sounds reasonable?

@rock3r
Copy link
Contributor

rock3r commented Mar 30, 2019

Hey, just ran into this with exactly the scenario Artur is describing... I'd much rather have the class in the same file as the top-level function that returns it since they don't make sense in isolation. Would PR the proposed change but I have zero bandwidth until late May :(

@henrikbarium
Copy link

henrikbarium commented Feb 3, 2020

I'd love to see this change as well.

It does not have to do with "utility" files really, but when moving to a more functional style of programming. It is often the case that you have a primary public top-level function in the file, and then some supporting functions or data structures. These data structures (data classes) are really just part of the primary top-level function's API, there is no other reason for them to exist. Putting them in another file would make the code harder to understand as you would not have their definitions available when looking at the function.

I think this rule comes from an object-oriented state of mind, where everything is wrapped in a class. In an OO world, you would not have for example a top-level parseExpression function, you would have a parseExpression-function wrapped in a ExpressionParser class. Then it makes sense for that class to match the filename. In the functional style, it is really the name of the function that should match the filename, not the name of (one of) its supporting data structures.

@BraisGabin
Copy link
Member

Idea to fix this problem (I had it too and I disabled this rule): If there's only one class and it's at the beginning of the file the File should be named after it. Otherwise ignore it's ok.

All the complains came from a file that is top-level function oriented. It have no sense to declare the "no important class" at the begging of the file.

@henrikbarium
Copy link

I would prefer being able to exclude data classes altogether. It is not uncommon that the data classes used by the top-level function are declared before the function itself is declared. Also, there could be multiple data classes used by the function.

@arturbosch
Copy link
Member

I agree with both of your suggestions ^^

@henrikbarium
Copy link

henrikbarium commented Jun 26, 2020

I did not realize this issue was closed, but I have to say that the solution chosen unfortunately does not solve the situation I was describing. People coming from a functional background are often mathematically trained and prefer declaring anything needed by a function before that function. For example, if there is a supporting data structure and a top-level function which uses it, the data class declaration would come first. The mustBeFirst addition does not handle such a situation. Again, I would prefer to be able to exclude data classes, enum classes and other "data structures" from the rule. The only exception I can see if a data or enum class is the only declaration in a file.

PS. I don't know what the comment etiquette says about comments on closed issues. Should I open a new issue for this comment?

@cortinico
Copy link
Member

PS. I don't know what the comment etiquette says about comments on closed issues. Should I open a new issue for this comment?

Ideally yes, please create a new issue.

The discussion in this one is more than 1 year old and people could have muted this thread.

Moreover, having also an example would help streamline the discussion and will clarify why mustBeFirst doesn't fit your needs.

@khaleelf
Copy link

khaleelf commented Jan 7, 2021

For what it's worth, I moved the data class to the bottom of the same file and it rectified the issue for me.

@cortinico
Copy link
Member

For what it's worth, I moved the data class to the bottom of the same file and it rectified the issue for me.

@khaleelf Please open a new issue with a snipped explaining your issue. We try to close issues that get stale and create new one to make them easier to handle.

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

No branches or pull requests

8 participants