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

[TECHNICAL] Detekt: static code analyzer #4487

Draft
wants to merge 32 commits into
base: master
Choose a base branch
from
Draft

Conversation

jesmrec
Copy link
Collaborator

@jesmrec jesmrec commented Oct 4, 2024

Related Issues

App: #4506

  • Add changelog files for the fixed issues in folder changelog/unreleased. More info here
  • Add feature to Release Notes in ReleaseNotesViewModel.kt creating a new ReleaseNote() with String resources (if required)

QA

@jesmrec jesmrec force-pushed the feature/detekt branch 7 times, most recently from a0c1d33 to e0fe17b Compare October 9, 2024 15:32
@jesmrec jesmrec force-pushed the feature/detekt branch 2 times, most recently from 6bfce71 to 340aebb Compare October 16, 2024 07:41
@jesmrec jesmrec force-pushed the feature/detekt branch 6 times, most recently from cd5e812 to 59aeb55 Compare October 30, 2024 08:23
@jesmrec jesmrec linked an issue Oct 30, 2024 that may be closed by this pull request
51 tasks
@jesmrec jesmrec force-pushed the feature/detekt branch 3 times, most recently from 94396a2 to f939d1f Compare October 30, 2024 16:20
@jesmrec jesmrec force-pushed the feature/detekt branch 2 times, most recently from 0836d61 to b96e246 Compare November 20, 2024 10:10
@JuancaG05 JuancaG05 changed the title [FEATURE] Detekt as static code analyzer [TECHNICAL] Detekt: static code analyzer Dec 2, 2024
Copy link
Collaborator

@JuancaG05 JuancaG05 left a comment

Choose a reason for hiding this comment

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

Some comments to take into account before we merge 😃 @jesmrec

gradle/libs.versions.toml Outdated Show resolved Hide resolved

push:
branches:
- feature/detekt
Copy link
Collaborator

Choose a reason for hiding this comment

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

We'll need to review this before merging, and probably remove it

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is totally intended at this point. That makes detekt to execute only in the current branch because the work is not finished. This will not be removed but replaced for a correct policy that we will agree in.

- feature/detekt
pull_request:
branches:
- "master"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably we want to run Detekt in every PR we do, not only in master (for which we cannot create PR), so this needs to be reviewed as well

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Same as before. Intended and provisional.

.github/workflows/detekt.yml Outdated Show resolved Hide resolved
.github/workflows/detekt.yml Outdated Show resolved Hide resolved
@JuancaG05
Copy link
Collaborator

JuancaG05 commented Dec 10, 2024

MaximumLineLength, from the "Formatting" section, is currently enabled. In the docs it says it overlaps with MaxLineLength, which we are relying on, and they have different values. Needs to be checked.

EDIT: "Formatting" section was not yet considered for this iteration, but we'll need to use the MaximumLineLength rule since it has the ignoreBackTickedIdentifier parameter, which allows to ignore the lines for the tests names with the backtick format, which we use and that currently exceed the line length limit (there is no way to break them in more than 1 line).

These reports will be avoided for the moment with @Suppress annotations, but once we enable MaximumLineLength they should be removed.

@JuancaG05
Copy link
Collaborator

Just checked (tested empirically) with the LongMethod rule that Detekt counts LLOC when talking about lines of code, and so I guess this happens with every rule that has to do with lines of code. That means, when trying to reduce lines of code, don't play with blank lines or comments since that won't make effect 😉

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.

[TECHNICAL] Detekt: static code analyzer
3 participants