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

Sort config lists #4014

Merged
merged 1 commit into from
Aug 11, 2021
Merged

Sort config lists #4014

merged 1 commit into from
Aug 11, 2021

Conversation

BraisGabin
Copy link
Member

In this release we are already changing our default config yaml. For that reason I think that we could use it to sort the list configurations to follow an alphabetic order. I sorted all of them but I'm not sure if it have sense to do it with all of them because maybe there is another "more logical order". These are the rules that maybe a "more logical order" can exist:

  • ComplexMethod (keep all the scope functions together)
  • ExceptionRaisedInUnexpectedLocation (order them for "more common")
  • ForbiddenComment (order them for "more common")

@codecov
Copy link

codecov bot commented Aug 7, 2021

Codecov Report

Merging #4014 (f622d26) into main (7540dc2) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##               main    #4014   +/-   ##
=========================================
  Coverage     83.55%   83.56%           
  Complexity     3181     3181           
=========================================
  Files           459      459           
  Lines          9085     9090    +5     
  Branches       1771     1771           
=========================================
+ Hits           7591     7596    +5     
  Misses          561      561           
  Partials        933      933           
Impacted Files Coverage Δ
...rturbosch/detekt/rules/complexity/ComplexMethod.kt 95.34% <100.00%> (ø)
...arturbosch/detekt/rules/bugs/IgnoredReturnValue.kt 83.33% <100.00%> (ø)
.../exceptions/ExceptionRaisedInUnexpectedLocation.kt 95.23% <100.00%> (+1.48%) ⬆️
...osch/detekt/rules/exceptions/SwallowedException.kt 74.54% <100.00%> (ø)
...eptions/ThrowingExceptionsWithoutMessageOrCause.kt 91.66% <100.00%> (ø)
...tekt/rules/exceptions/TooGenericExceptionCaught.kt 87.50% <100.00%> (ø)
...tekt/rules/exceptions/TooGenericExceptionThrown.kt 59.09% <100.00%> (ø)
.../arturbosch/detekt/rules/style/ForbiddenComment.kt 100.00% <100.00%> (ø)
...turbosch/detekt/rules/style/ForbiddenMethodCall.kt 92.10% <100.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 7540dc2...f622d26. Read the comment docs.

@cortinico cortinico added this to the 1.18.0 milestone Aug 7, 2021
@cortinico cortinico added the housekeeping Marker for housekeeping tasks and refactorings label Aug 7, 2021
Copy link
Member

@schalkms schalkms left a comment

Choose a reason for hiding this comment

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

I'm not sure. We would have to pay attention. Every PR could change the alphabetical order. I don't think this would improve anything.

@cortinico
Copy link
Member

Every PR could change the alphabetical order.

I was thinking about the same. Perhaps we should consider adding some pre-commit hooks or PR checks that verifies the YAML files?

@schalkms
Copy link
Member

schalkms commented Aug 7, 2021

A pre-commit hook doesn't help much, since every contributor needs to configure the hook on the local machine.
This additional step makes it harder for every potential contributor to actually contribute to the project. Moreover, a pre-commit hook works only locally on the dev machine.
A PR check or even better a unit test could help, but I am unsure regarding value/effort.

@BraisGabin
Copy link
Member Author

I'm not sure. We would have to pay attention.

Completely agree. This is easy to be broken. I do this change just because we are changing all those lines anyway so, at least, have an alphabetical order for some configurations that, usually, doesn't change. For me the check/unit test is not worth it. I vote to merge because this lines will be changed for all our users anyway. And if a next change "breaks" the order... well, that's not a big deal.

@BraisGabin BraisGabin merged commit 66049c5 into main Aug 11, 2021
@BraisGabin BraisGabin deleted the sort-config-lists branch August 11, 2021 15:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
housekeeping Marker for housekeeping tasks and refactorings
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants