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

Fix ModifierOrder to support value class #3719

Merged
merged 1 commit into from May 23, 2021

Conversation

eygraber
Copy link
Contributor

On top of #3718

Fixes #3717

@eygraber eygraber force-pushed the fix-modifier-order-value-class branch from 593a288 to cb1758b Compare April 29, 2021 21:04
@codecov
Copy link

codecov bot commented Apr 30, 2021

Codecov Report

Merging #3719 (d25098e) into main (22eec63) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##               main    #3719   +/-   ##
=========================================
  Coverage     83.58%   83.58%           
  Complexity     3055     3055           
=========================================
  Files           456      456           
  Lines          8916     8917    +1     
  Branches       1752     1752           
=========================================
+ Hits           7452     7453    +1     
  Misses          541      541           
  Partials        923      923           
Impacted Files Coverage Δ Complexity Δ
...lab/arturbosch/detekt/rules/style/ModifierOrder.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 22eec63...d25098e. Read the comment docs.

@eygraber eygraber force-pushed the fix-modifier-order-value-class branch 9 times, most recently from a9064d9 to d2b87af Compare May 12, 2021 18:15
@eygraber eygraber force-pushed the fix-modifier-order-value-class branch 2 times, most recently from 566bc32 to 2d5da7e Compare May 21, 2021 03:35
@eygraber eygraber force-pushed the fix-modifier-order-value-class branch from 2d5da7e to d25098e Compare May 21, 2021 06:31
@cortinico cortinico removed the blocked label May 21, 2021
@cortinico cortinico added this to the 1.18.0 milestone May 21, 2021
@@ -75,6 +76,7 @@ class ModifierOrder(config: Config = Config.empty) : Rule(config) {
ENUM_KEYWORD, ANNOTATION_KEYWORD, FUN_KEYWORD,
COMPANION_KEYWORD,
INLINE_KEYWORD,
VALUE_KEYWORD,
Copy link
Member

Choose a reason for hiding this comment

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

It seems reasonable to put value here because value class is inline class. However, would you mind creating a PR for Kotlin coding convention here since it does not include the value modifier yet?

Copy link
Member

Choose a reason for hiding this comment

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

I just created that PR: JetBrains/kotlin-web-site#2300

Should we wait for it to be merged before merging this? Or can we merge it and revert it in case it is rejected?

Copy link
Member

Choose a reason for hiding this comment

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

I am inclined to merge it now since this will impact the correctness when running Detekt on Kotlin 1.5 code. See https://github.com/detekt/detekt/pull/3810/files#r637354618

@chao2zhang chao2zhang merged commit df4d921 into detekt:main May 23, 2021
@netroy
Copy link

netroy commented Jun 22, 2021

@cortinico @chao2zhang can we please have a new release that'd include this fix?

@cortinico cortinico changed the title Fix modifier order value class Fix ModifierOrder to support value class Jul 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

False positive on ModifierOrder for value classes in Kotlin 1.5
5 participants