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
Remove existing rule MandatoryBracesIfStatements #2962
Comments
The suggestion is good. However, this rule needs practically 0 maintenance effort. |
That's not totally true, as a couple of months ago a false negative had to be fixed for this rule (#2835). It can be imagined that a future case arises again.
Maybe I'm missing something, but I don't see the issue with deprecating MandatoryBracesIfStatements and telling the users to replace its usage with MultiLineIfElse.
From the talk by @Mauin, he says that all formatting rules had been stripped from detekt as ktlint does a really good job with that already. Even though I know this can be a tentative issue, but I'd love to have some further discussions before being closed. |
This information is a bit outdated. Previously, detekt had it’s own formatting rules that messed around with user code. The detekt team decided to wrap ktlint and remove rules that were manipulating user code. Please see it this way. The user has the choice, whether user code should be replaced or not.
The rules serve kind of a different use case as mentioned above. The choice is up to the user. |
But the
As both of these rules do the exact same thing, I don't see how it matters what rulesets each of them belongs to.
I understand, but that's why there is the
The way I see it is that @arturbosch @BraisGabin @cortinico Maybe I'm missing something here, do you care to chime in? |
It’s only available in the formatting ruleset, which entirely wraps ktlint rules. autocorrect is just available for this ruleset.
The formatting ruleset consists entirely of ktlint rules. The style ruleset on the other hand consists of detekt native rules.
Because ktlint automatically formats user code according to the guidelines. Detekt doesn’t do that. That’s the difference. Ktlint is a linter, detekt a static analysis tool. Both tools serve a different purpose, but integrate nicely with each other. |
I hope that I could clear out all doubts and answer all questions @veyndan |
Hmm, I feel that you're just explaining the status quo to me (which I already understand). Let me rephrase the question: Why would I (a detekt user) prefer |
Okay. Let me answer the following question.
If a user doesn’t want to use the ktlint wrapper or has ktlint as a separate tool in use, The same holds true for the |
Thanks for the answer! Just a couple of further questions on your answer.
Surely if they are using ktlint as a separate tool (and they have enabled
Why would a user not want to use the ktlint wrapper and use detekt in isolation (i.e., without ktlint as a separate tool)? |
I think that, right now, we can't remove it. My reasons:
Other duplicated rules detekt/ktlint:
@schalkms what do you think about reconsider this for 2.0? Good thing: Now that I know that ktlin implemented this rule I'll reported them the case that I fixed in #2835 because, at least, they don't have any test to ensure that such case is ok. |
Thanks @BraisGabin! Those reasons make sense. I'd be happy with reopening this issue and adding it to the 2.0 milestone 😄 |
If detekt is that far we can reconsider or reopen this ofc. At the moment, this is not the case due to all of the mentioned points. That's why I closed it. |
A linter has a different purpose compared to a static analysis tool. A linter most likely runs only on the client, whereas a static analysis tool should run on the CI, if there's any. Detekt has a reporting functionality that ktlint has not.
Brais already answered this. Additionally, manipulating user code can always lead to unwanted results. |
Okay, let's have an example. You run ktlint on the client with
In this case, it is an experimental rule (even though in detekt we don't differentiate that in the documentation). What about a non-experimental rule like
I've already responded to this above regarding enabling or disabling |
I moved this idea to #2680 where we have all the ideas for 2.0 |
Why did you enable ktlint as a linter on your CI? |
You mentioned in the last comment that you enabled ktlint and detekt on the CI. Added bold for emphasis:
This just furthers the question that I've been asking all this time: if activating both detekt rules is redundant, then why should we have both detekt rules (in the long run)?
I enabled The second reason is maintainability of the project and sharing knowledge between ktlint and detekt. Some projects I work with only use ktlint, while others use detekt. If I (or anyone else) finds a bug in ktlint, I'd love to see it propagated to detekt instead of having to make another PR for the duplicate detekt rule. |
Thanks for the respons!
I removed it in order to sort out further confusion. This is what I meant regarding the tool usage. Whilst both rules might do the same, the inner workings of both rules are different. The ktlint rule does only consider mandatory braces for if statements but not for loops. As a side-note, I also understand that it might be confusing. |
To conclude this conversation, both mentioned rules don't work the same. |
Ah must've missed that comment! @BraisGabin said in #2680 (comment) that he'll create an issue on ktlint to resolve this. I think the other rules that @BraisGabin mentioned in #2680 (comment) still apply though, I just happened to choose an experimental rule that is doing stuff out of the scope of its name 😅 . I agree that we should conclude the discussion though as it's getting pretty long 😂 As it's already linked in #2680, when the talks about detekt 2.0 come around, we can resume the conversation then if that's good for you. |
The issue: pinterest/ktlint#828 And as @schalkms say it seems for pinterest/ktlint#812 if (true)
50
else
55 So I imagine that they will rollback that part of the rule 🤷♂️. They will probably maintain it for |
It is really unfortunately for the user that there are similar/identical rules. I think we can improve the docs further to help them here.
Why you might prefer There is often a overlap of rules between tools e.g. pmd, checkstyle and sonarqube have rules which detect classic code smells like LongMethod's and LongParameterLists. Sonarqube allows to include checkstyle and pmd as plugins. Should sonar remove it's core rules for the others? No, they may also address different users. What we can do is some kind of mapping. If a detekt rule detects something similar as a KtLint we only report once. |
The rules
MandatoryBracesIfStatements
andMultiLineIfElse
appear to achieve the same thing. I'd nominate to removeMandatoryBracesIfStatements
overMultiLineIfElse
becauseMultiLineIfElse
is maintained by ktlint (which avoids duplication in improvements/bug fixes) andMultiLineIfElse
has the possibility for auto-correction.The text was updated successfully, but these errors were encountered: