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

MODELIX-794: adapt the detekt configuration according to the discussions from the coding conventions workshop #670

Conversation

languitar
Copy link
Contributor

Updates detekt to:

  • lint for missing docs on public members
  • lint for usage of !! to prefer checkNotNull or requireNotNull

I've also fixed a bug in CI where multiplatform code was not linted due to the detekt target not including multiplatform targets.

Copy link

github-actions bot commented Apr 4, 2024

Test Results

122 files  +1  122 suites  +1   8m 0s ⏱️ +39s
736 tests  - 2  728 ✅  - 2  8 💤 ±0  0 ❌ ±0 
746 runs   - 2  738 ✅  - 2  8 💤 ±0  0 ❌ ±0 

Results for commit 688bed5. ± Comparison against base commit 19e7e52.

This pull request removes 3 and adds 1 tests. Note that renamed tests count towards both.
LinearHistoryTest ‑ knownPerformanceIssue[js, browser]
LinearHistoryTest ‑ knownPerformanceIssue[js, node]
LinearHistoryTest ‑ knownPerformanceIssue[jvm]
org.modelix.detekt.AvoidNonNullAssertionOperatorTest ‑ find double bang operator()

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Apr 4, 2024

JVM coverage report

Overall Project 46.59% -0.01%
Files changed 76.39%

File Coverage
AvoidNonNullAssertionOperator.kt 100% 🍏
RuleSetProvider.kt 0%

In multiplatform modules, the detekt task does not include tasks for JVM
and JS source sets.
GitHub only allows 20 SARIF runs in a single upload. However, our number
of modules already exceeds this limit. Therefore, we combine the
resulting detekt sarif files into a single file with only a single run,
which circumvents this limit.
@languitar languitar force-pushed the feature/MODELIX-794-Adapt-the-detekt-configuration-according-to-the-discussions-from-the-coding-conventions-workshop branch 2 times, most recently from aa0c47a to 7116244 Compare April 18, 2024 12:40
@languitar languitar marked this pull request as ready for review April 18, 2024 12:53
@languitar languitar requested review from slisson and a team as code owners April 18, 2024 12:53
import io.gitlab.arturbosch.detekt.api.Severity
import org.jetbrains.kotlin.psi.KtReferenceExpression

class AvoidNonNullAssertionOperator(config: Config) : Rule(config) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting how custom rules can be added.

But according to detekt/detekt#4832 it can be replaced by UnnecessaryNotNullOperator and UnsafeCallOnNullableType

Copy link
Contributor Author

Choose a reason for hiding this comment

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

UnsafeCallOnNullableType is active by default but doesn't catch all of the cases the custom rule is reporting on. For whatever reason....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just tried it again, even with actively enabling those two rules, despite the fact that they should be active by default, I can't get a single report from them. Not sure if that's a detekt bug.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thist might be realted to type resolution.

See detekt/detekt#2036 (comment)
See https://detekt.github.io/detekt/type-resolution.html

@languitar languitar force-pushed the feature/MODELIX-794-Adapt-the-detekt-configuration-according-to-the-discussions-from-the-coding-conventions-workshop branch from f60c055 to 8f9aa0c Compare April 25, 2024 07:05
The `knownPerformanceIssue` test case was now constantly timing out when
being executed in JS. After clarification, all corner cases it included
are already covered by the remaining tests in the test file and also by
MergeOrderTest, ConflictResolutionTest, and UndoTest. Therefore, I have
dropped this test case to avoid the timeout and to reduce the amount of
hard to maintain test cases.

The expensive part about the test was the way assertHistoryIsCorrect was
implemented, not the production code.
@languitar languitar force-pushed the feature/MODELIX-794-Adapt-the-detekt-configuration-according-to-the-discussions-from-the-coding-conventions-workshop branch from 8f9aa0c to 688bed5 Compare April 25, 2024 07:06
@languitar languitar merged commit 854f72c into main Apr 25, 2024
14 checks passed
@languitar languitar deleted the feature/MODELIX-794-Adapt-the-detekt-configuration-according-to-the-discussions-from-the-coding-conventions-workshop branch April 25, 2024 07:16
@slisson
Copy link
Member

slisson commented Apr 26, 2024

🎉 This PR is included in version 7.3.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

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.

None yet

3 participants