build: add Spotless, Detekt, Lint baseline, Konsist, pre-commit hook#129
Open
Lemkinator wants to merge 7 commits into
Open
build: add Spotless, Detekt, Lint baseline, Konsist, pre-commit hook#129Lemkinator wants to merge 7 commits into
Lemkinator wants to merge 7 commits into
Conversation
Step 2 of the modernization plan (config/spotless/apache-2.0.kt uses 2023, this app's first-commit year, as the copyright start).
Stamps Apache 2.0 license headers on Kotlin/XML/gradle sources and applies ktlint formatting repo-wide (Step 2 of the modernization plan). Also drops the deprecated AGP lint sarifReport/htmlReport toggles and adds the ktlint header delimiter regex Spotless 8.8 requires for kotlinGradle/xml targets.
- Replace bare e.printStackTrace() with Log.e(tag, message, e) across provider/use-case/UI catch blocks so stack traces reach logcat instead of stdout, and suppress TooGenericExceptionCaught with justification where narrowing the catch type isn't practical (network/JSON parsing). - Expand wildcard imports (de.lemke.oneurl.data.database.*, androidx.room.*). - Rename domainMapper.kt -> DomainMapper.kt (ktlint standard:filename). - Rename RequestQueueSingleton's INSTANCE -> instance (ktlint property-naming). Step 2 of the modernization plan.
…ileTree Root build.gradle.kts and settings.gradle.kts were never linted or license-checked since app's kotlinGradle target only resolved relative to app/. Apply Spotless at root with a fileTree(rootDir) covering every *.gradle.kts in the repo, and drop the now-redundant block from app/build.gradle.kts. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…line
- Extract inline Volley success/error response handling into named
private functions across provider getCreateRequest implementations
(L4f, Murl, Oneptco, Shareaholic, Spoome, Ulvis, VgdIsgd) and
CheckURLSafetyUseCase, resolving CyclomaticComplexMethod/LongMethod.
- Same extraction for AddURLActivity.showErrorDialog,
GenerateQRCodeActivity.initControls, ProviderInfoBottomSheet.onViewCreated,
and URLActivity.onOptionsItemSelected/collectState.
- Collapse ShortURLProviderCompanion's class+companion-object wrapper into
a plain object (UtilityClassWithPublicConstructor).
- Simplify URL.contains()'s multi-field OR chain into a list + any {}
(ComplexCondition).
- Configure ReturnCount (max 4, excludeGuardClauses) — the four flagged
functions are idiomatic guard-clause/when-per-branch Android code, not
actual control-flow smells.
- Snapshot app/lint-baseline.xml (18 pre-existing warnings not introduced
by this PR).
Resolves all 133 Detekt findings from the initial rollout. Step 2 of the
modernization plan.
Enforces data/domain/ui layering (data may depend on domain.model — the shared URL/ShortURLProvider value types — but not on use cases or other domain logic), use-case operator invoke, ViewModel inheritance, and @hiltviewmodel constructor injection. Kotest is deliberately deferred to Plan 4 (unit test scaffolding) rather than pulled in early for this alone — plain JUnit5 + Konsist is sufficient here and avoids installing test dependencies ahead of the step that actually consumes them. dependency-analysis buildHealth report is clean (no unused/misconfigured dependencies). Step 4 of the modernization plan.
- .gitattributes enforces LF line endings (CRLF breaks Spotless). - .githooks/pre-commit blocks commits with Spotless/Detekt violations; fails fast if core.autocrlf=true is detected. - Document the one-time hooksPath opt-in in README.md and CLAUDE.md. Step 5 of the modernization plan.
Contributor
|
Important Review skippedToo many files! This PR contains 107 files, which is 7 over the limit of 100. To get a review, narrow the scope: Upgrade to a paid plan to raise the limit. This review couldn't start because sufficient usage credits or metered capacity aren't available. Add credits or update usage-based reviews in the billing tab, then retry. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (108)
You can disable this status message by setting the ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Owner
Author
|
| Filename | Overview |
|---|---|
| app/src/test/java/de/lemke/oneurl/ArchitectureTest.kt | New Konsist architecture tests enforcing layering, use-case invoke, ViewModel inheritance, and Hilt constructor injection — well-structured and correct; scope initialized at field level which is standard Konsist practice |
| app/src/main/java/de/lemke/oneurl/ui/AddURLActivity.kt | Refactored error dialog logic into named extension functions; simpleErrorMessageRes uses a when expression with an else branch over a sealed class, silently dropping compiler exhaustiveness enforcement |
| config/detekt/detekt.yml | Reasonable Detekt configuration; disables MagicNumber and TooManyFunctions, raises MaxLineLength to 140, excludes common benign exception types from SwallowedException |
| .githooks/pre-commit | Runs spotlessCheck and detekt before commits; includes CRLF guard for Windows users and clear remediation instructions |
| app/build.gradle.kts | Adds Spotless, Detekt, JUnit 5 platform, Konsist test deps, and lint configuration with baseline; useJUnitPlatform() correctly wired for all unit tests |
| gradle/libs.versions.toml | Adds versions and aliases for Detekt, Spotless, Konsist, JUnit Jupiter 6.1.2, dependency-analysis, and ktlint (version-only ref); existing versions unchanged |
| app/src/main/java/de/lemke/oneurl/domain/model/ShortURLProvider.kt | Companion class correctly converted to a top-level object; all callers updated; ktlint-style comment formatting fixes applied throughout |
| app/src/main/java/de/lemke/oneurl/data/database/DomainMapper.kt | Renamed from domainMapper.kt to DomainMapper.kt (PascalCase); license header added; functionally identical |
| app/src/main/java/de/lemke/oneurl/domain/CheckURLSafetyUseCase.kt | Response-parsing logic extracted into parseUrlhausResponse and buildBlacklistedResult; logic is semantically equivalent to the original and improves readability |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[AddURLActivity\nshowErrorDialog] --> B{configureFor}
B -->|NoInternet| C[configureNoInternet\nopen wireless settings]
B -->|BlacklistedURL| D[configureBlacklisted\nURLhaus / VirusTotal buttons]
B -->|ServiceTemporarilyUnavailable| E[configureServiceUnavailable\nMore Information button]
B -->|Custom| F[configureCustom\nstatus code + message]
B -->|Unknown| G[configureUnknown\nstatus code or generic text]
B -->|else| H[simpleErrorMessageRes]
H --> I[R.string resource\nfor simple data-object errors]
H -->|else catch-all| J[commonutils_error_unknown]
style J fill:#ffe0b2,stroke:#e65100
style B fill:#e3f2fd,stroke:#1565c0
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
app/src/main/java/de/lemke/oneurl/ui/AddURLActivity.kt:246-258
The `when` here is used as an expression (returns `Int`), so Kotlin normally enforces exhaustiveness over sealed classes — but the `else` branch opts out of that guarantee. If a new simple `GenerateURLError` subtype is added in the future, it will silently resolve to a generic "unknown error" string with no compile-time warning. Removing `else` and listing the few cases that "shouldn't" reach here (or using `is GenerateURLError.Unknown`, `is GenerateURLError.Custom`, etc. to absorb them) would restore compiler-enforced exhaustiveness.
```suggestion
private fun simpleErrorMessageRes(error: GenerateURLError): Int =
when (error) {
GenerateURLError.AliasAlreadyExists -> R.string.error_alias_already_exists
GenerateURLError.URLExistsWithDifferentAlias -> R.string.error_url_already_exists_with_different_alias
GenerateURLError.InvalidURL -> R.string.error_invalid_url
GenerateURLError.InvalidAlias -> R.string.error_invalid_alias
GenerateURLError.InvalidURLOrAlias -> R.string.error_invalid_url_or_alias
GenerateURLError.InternalServerError -> R.string.error_internal_server_error
GenerateURLError.ServiceOffline -> R.string.error_service_offline
GenerateURLError.RateLimitExceeded -> R.string.error_rate_limit_exceeded
GenerateURLError.DomainNotAllowed -> R.string.error_domain_not_allowed
// Complex types handled by configureFor; should never reach here
is GenerateURLError.Unknown,
is GenerateURLError.Custom,
is GenerateURLError.BlacklistedURL,
is GenerateURLError.ServiceTemporarilyUnavailable,
GenerateURLError.NoInternet,
-> commonutilsR.string.commonutils_error_unknown
}
```
### Issue 2 of 2
app/src/test/java/de/lemke/oneurl/ArchitectureTest.kt:63-67
`hasParent { parent -> parent.name == "ViewModel" }` checks the **direct** parent class name. Any ViewModel that extends `AndroidViewModel` (or another intermediate subclass) instead of `ViewModel` directly would fail this test even though it legitimately is a ViewModel. All current ViewModels extend `ViewModel()` directly so the tests pass today, but the rule is brittle for future additions — consider checking `it.hasParentWithName("ViewModel") || it.hasParentWithName("AndroidViewModel")` to future-proof it.
Reviews (1): Last reviewed commit: "build: add .gitattributes, pre-commit ho..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+246
to
+258
| private fun simpleErrorMessageRes(error: GenerateURLError): Int = | ||
| when (error) { | ||
| GenerateURLError.AliasAlreadyExists -> R.string.error_alias_already_exists | ||
| GenerateURLError.URLExistsWithDifferentAlias -> R.string.error_url_already_exists_with_different_alias | ||
| GenerateURLError.InvalidURL -> R.string.error_invalid_url | ||
| GenerateURLError.InvalidAlias -> R.string.error_invalid_alias | ||
| GenerateURLError.InvalidURLOrAlias -> R.string.error_invalid_url_or_alias | ||
| GenerateURLError.InternalServerError -> R.string.error_internal_server_error | ||
| GenerateURLError.ServiceOffline -> R.string.error_service_offline | ||
| GenerateURLError.RateLimitExceeded -> R.string.error_rate_limit_exceeded | ||
| GenerateURLError.DomainNotAllowed -> R.string.error_domain_not_allowed | ||
| else -> commonutilsR.string.commonutils_error_unknown | ||
| } |
There was a problem hiding this comment.
when here is used as an expression (returns Int), so Kotlin normally enforces exhaustiveness over sealed classes — but the else branch opts out of that guarantee. If a new simple GenerateURLError subtype is added in the future, it will silently resolve to a generic "unknown error" string with no compile-time warning. Removing else and listing the few cases that "shouldn't" reach here (or using is GenerateURLError.Unknown, is GenerateURLError.Custom, etc. to absorb them) would restore compiler-enforced exhaustiveness.
Suggested change
| private fun simpleErrorMessageRes(error: GenerateURLError): Int = | |
| when (error) { | |
| GenerateURLError.AliasAlreadyExists -> R.string.error_alias_already_exists | |
| GenerateURLError.URLExistsWithDifferentAlias -> R.string.error_url_already_exists_with_different_alias | |
| GenerateURLError.InvalidURL -> R.string.error_invalid_url | |
| GenerateURLError.InvalidAlias -> R.string.error_invalid_alias | |
| GenerateURLError.InvalidURLOrAlias -> R.string.error_invalid_url_or_alias | |
| GenerateURLError.InternalServerError -> R.string.error_internal_server_error | |
| GenerateURLError.ServiceOffline -> R.string.error_service_offline | |
| GenerateURLError.RateLimitExceeded -> R.string.error_rate_limit_exceeded | |
| GenerateURLError.DomainNotAllowed -> R.string.error_domain_not_allowed | |
| else -> commonutilsR.string.commonutils_error_unknown | |
| } | |
| private fun simpleErrorMessageRes(error: GenerateURLError): Int = | |
| when (error) { | |
| GenerateURLError.AliasAlreadyExists -> R.string.error_alias_already_exists | |
| GenerateURLError.URLExistsWithDifferentAlias -> R.string.error_url_already_exists_with_different_alias | |
| GenerateURLError.InvalidURL -> R.string.error_invalid_url | |
| GenerateURLError.InvalidAlias -> R.string.error_invalid_alias | |
| GenerateURLError.InvalidURLOrAlias -> R.string.error_invalid_url_or_alias | |
| GenerateURLError.InternalServerError -> R.string.error_internal_server_error | |
| GenerateURLError.ServiceOffline -> R.string.error_service_offline | |
| GenerateURLError.RateLimitExceeded -> R.string.error_rate_limit_exceeded | |
| GenerateURLError.DomainNotAllowed -> R.string.error_domain_not_allowed | |
| // Complex types handled by configureFor; should never reach here | |
| is GenerateURLError.Unknown, | |
| is GenerateURLError.Custom, | |
| is GenerateURLError.BlacklistedURL, | |
| is GenerateURLError.ServiceTemporarilyUnavailable, | |
| GenerateURLError.NoInternet, | |
| -> commonutilsR.string.commonutils_error_unknown | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/src/main/java/de/lemke/oneurl/ui/AddURLActivity.kt
Line: 246-258
Comment:
The `when` here is used as an expression (returns `Int`), so Kotlin normally enforces exhaustiveness over sealed classes — but the `else` branch opts out of that guarantee. If a new simple `GenerateURLError` subtype is added in the future, it will silently resolve to a generic "unknown error" string with no compile-time warning. Removing `else` and listing the few cases that "shouldn't" reach here (or using `is GenerateURLError.Unknown`, `is GenerateURLError.Custom`, etc. to absorb them) would restore compiler-enforced exhaustiveness.
```suggestion
private fun simpleErrorMessageRes(error: GenerateURLError): Int =
when (error) {
GenerateURLError.AliasAlreadyExists -> R.string.error_alias_already_exists
GenerateURLError.URLExistsWithDifferentAlias -> R.string.error_url_already_exists_with_different_alias
GenerateURLError.InvalidURL -> R.string.error_invalid_url
GenerateURLError.InvalidAlias -> R.string.error_invalid_alias
GenerateURLError.InvalidURLOrAlias -> R.string.error_invalid_url_or_alias
GenerateURLError.InternalServerError -> R.string.error_internal_server_error
GenerateURLError.ServiceOffline -> R.string.error_service_offline
GenerateURLError.RateLimitExceeded -> R.string.error_rate_limit_exceeded
GenerateURLError.DomainNotAllowed -> R.string.error_domain_not_allowed
// Complex types handled by configureFor; should never reach here
is GenerateURLError.Unknown,
is GenerateURLError.Custom,
is GenerateURLError.BlacklistedURL,
is GenerateURLError.ServiceTemporarilyUnavailable,
GenerateURLError.NoInternet,
-> commonutilsR.string.commonutils_error_unknown
}
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+63
to
+67
| .functions(includeNested = false, includeLocal = false) | ||
| .any { it.name == "invoke" && it.hasModifier(KoModifier.OPERATOR) } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
hasParent { parent -> parent.name == "ViewModel" } checks the direct parent class name. Any ViewModel that extends AndroidViewModel (or another intermediate subclass) instead of ViewModel directly would fail this test even though it legitimately is a ViewModel. All current ViewModels extend ViewModel() directly so the tests pass today, but the rule is brittle for future additions — consider checking it.hasParentWithName("ViewModel") || it.hasParentWithName("AndroidViewModel") to future-proof it.
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/src/test/java/de/lemke/oneurl/ArchitectureTest.kt
Line: 63-67
Comment:
`hasParent { parent -> parent.name == "ViewModel" }` checks the **direct** parent class name. Any ViewModel that extends `AndroidViewModel` (or another intermediate subclass) instead of `ViewModel` directly would fail this test even though it legitimately is a ViewModel. All current ViewModels extend `ViewModel()` directly so the tests pass today, but the rule is brittle for future additions — consider checking `it.hasParentWithName("ViewModel") || it.hasParentWithName("AndroidViewModel")` to future-proof it.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Steps 2, 4, 5 of the modernization plan: static analysis tooling, architecture rules, and pre-commit enforcement.
config/detekt/detekt.yml), Apache 2.0 license headers stamped repo-wide, IDE↔CLI sync via.idea/ktlint-plugin.xml.printStackTrace→Log.e, wildcard imports, ktlint naming) plus structural refactors extracting complex Volley response-handling and UI dispatch logic (AddURLActivity,URLActivity,GenerateQRCodeActivity,ProviderInfoBottomSheet, providergetCreateRequestimplementations) into named private functions — no behavior changes.app/lint-baseline.xml) — 18 pre-existing warnings grandfathered in, not introduced by this PR.ArchitectureTest.kt) — enforcesdata/domain/uilayering (datamay depend ondomain.model's shared value types, never on use cases), use-caseoperator invoke, ViewModel inheritance,@HiltViewModelconstructor injection. Deliberately plain JUnit5 rather than Kotest — full unit-test scaffolding lands in Plan 4.buildHealthwired, report is clean..githooks/pre-commit) — blocks commits with Spotless/Detekt violations; documented opt-in in README/CLAUDE.md..gitattributesenforces LF line endings.Test plan
./gradlew spotlessCheck— green./gradlew detekt— green (0 findings)./gradlew lintDebug— green (baseline filters 18 pre-existing warnings)./gradlew assembleDebug— green./gradlew test— green (includes Konsist architecture tests)./gradlew buildHealth— clean report🤖 Generated with Claude Code