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

Adds a ForbiddenSingleExpressionSyntax rule #3550

Conversation

marschwar
Copy link
Contributor

closes #1675

While trying to find a reference for the rule documentation, I realized that there does not seem to be agreement about this in the community. I guess this will never become a rule that is turned on be default.

While I personally like the rule, I am wondering if detekt should even be so opinionated about things that are not part of the official coding conventions or guidelines.

And then I may have formatting issues (again). Opening any other rule class causes the ktlint plugin to report an error such as Unexpected indentation (20) (should be 12) (indent). Any help will be greatly appreciated.

@codecov
Copy link

codecov bot commented Mar 13, 2021

Codecov Report

Merging #3550 (f3d4f2e) into master (bdceacb) will increase coverage by 0.00%.
The diff coverage is 82.60%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #3550   +/-   ##
=========================================
  Coverage     77.53%   77.54%           
- Complexity     2829     2839   +10     
=========================================
  Files           464      465    +1     
  Lines          8769     8790   +21     
  Branches       1713     1719    +6     
=========================================
+ Hits           6799     6816   +17     
  Misses         1046     1046           
- Partials        924      928    +4     
Impacted Files Coverage Δ Complexity Δ
...osch/detekt/rules/exceptions/SwallowedException.kt 77.35% <ø> (ø) 19.00 <0.00> (ø)
...t/rules/style/ForbiddenSingleExpressionFunction.kt 80.00% <80.00%> (ø) 10.00 <10.00> (?)
...n/io/github/detekt/report/sarif/RuleDescriptors.kt 20.00% <100.00%> (ø) 0.00 <0.00> (ø)
...rturbosch/detekt/rules/style/StyleGuideProvider.kt 100.00% <100.00%> (ø) 3.00 <0.00> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bdceacb...609f4b5. Read the comment docs.

@cortinico cortinico added this to the 1.17.0 milestone Mar 14, 2021
@BraisGabin
Copy link
Member

While I personally like the rule, I am wondering if detekt should even be so opinionated about things that are not part of the official coding conventions or guidelines.

We talked about this a lot of times in different issues. We need a controversial ruleset. But I also like this rule a lot. And I know other devs that don't like the overuse of this feature either.

And then I may have formatting issues (again). Opening any other rule class causes the ktlint plugin to report an error such as Unexpected indentation (20) (should be 12) (indent). Any help will be greatly appreciated.

We can open a new issue about this topix. I think that we should enable ktlint in this project. I have the same issue autoformatting some rules that it changes far too much lines.

Debt.FIVE_MINS
)

@Suppress("ReturnCount")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This suppression shouldn't be necessary, since the guard clause having a return statement are excluded.

@schalkms
Copy link
Member

We need a controversial ruleset.

I suggested this in a few PRs as well. +1 from my side.

@marschwar marschwar force-pushed the feature/1657-forbidden-single-expression-syntax branch 2 times, most recently from 9b9f2df to c510158 Compare March 14, 2021 20:20
@marschwar
Copy link
Contributor Author

How about I at least add a @controversial to the kdoc?

@BraisGabin
Copy link
Member

BraisGabin commented Mar 15, 2021

Great idea!!! Yes! We can add the rules in any ruleset but we can mark them as controversial! I think that we can make a new issue to implement this idea and merge this directly. The annotation is a better solution and it is not a breaking change so we can even mark old rules. Win-win!

@marschwar marschwar force-pushed the feature/1657-forbidden-single-expression-syntax branch 2 times, most recently from 53198e5 to 609f4b5 Compare March 16, 2021 06:32
@marschwar
Copy link
Contributor Author

I think this is obsolete as there is already a ImplicitUnitReturnType rule that checks the exact same case.

@marschwar marschwar closed this Mar 16, 2021
@marschwar marschwar deleted the feature/1657-forbidden-single-expression-syntax branch March 16, 2021 07:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Disallow single expression syntax when the function returns Unit
4 participants