Skip to content
This repository has been archived by the owner on Nov 1, 2022. It is now read-only.

Update detekt and its configuration #11540

Closed
wants to merge 8 commits into from
Closed

Update detekt and its configuration #11540

wants to merge 8 commits into from

Conversation

BraisGabin
Copy link
Contributor

As commented on mozilla-mobile/fenix#23123 this PR cleans a bit the current config, sync it with the default config of detekt and then update detekt along with it's configuration.

The sync is a manual work but the update of the config is done using these scripts: detekt/detekt#3558 (comment) (It had conflicts so there was a manual component there too).

I recommend to review this PR commit by commit because the merge of all the changes is a mess but each commit does one and only one thing so it should be easier to follow.


The new version of detekt found some new issues of already enabled issues (I assume they were old false-negatives) So I added them to the baseline. For that reason before adding those to the baseline I updated it.

@BraisGabin BraisGabin requested a review from a team as a code owner January 13, 2022 19:36
@BraisGabin
Copy link
Contributor Author

I have no idea what's wrong here. I can't see any log with the errors that CI found.

@gabrielluong
Copy link
Member

Looks like some of the CI task hanged. Closing and reopening the PR to re-run things.

@gabrielluong gabrielluong reopened this Jan 17, 2022
@gabrielluong gabrielluong self-requested a review January 18, 2022 23:17
@gabrielluong gabrielluong self-assigned this Jan 18, 2022
@gabrielluong
Copy link
Member

@BraisGabin Just for my own reference when reviewing and comparing the changes to detek.yml. Can I assuming you are adding the missing defaults that are seen in https://github.com/detekt/detekt/blob/main/detekt-core/src/main/resources/default-detekt-config.yml?

@BraisGabin
Copy link
Contributor Author

BraisGabin commented Jan 19, 2022

Yes, that's exactly what I did.

Edit: to be more precise. I used the config that is generated by detektGenerateConfig that is the same as this one: https://github.com/detekt/detekt/blob/v1.19.0/detekt-core/src/main/resources/default-detekt-config.yml (same file but the version from the tag)

@gabrielluong
Copy link
Member

Sorry for the ongoing delay on the review, will try to get to this soon!

@BraisGabin
Copy link
Contributor Author

Is there something I can do to help you reviewing this PR? I know it's big.

@gabrielluong
Copy link
Member

@BraisGabin I am so sorry for the delay. Other priorities took me hostage and kept me from investing the needed time to look at this thoroughly. I went through comparing your work, all our prior changes and the original config with the latest defaults and add some changes to try to align as closely with the defaults as possible. I made 2 minor changes in #11867 (which is where I will try to land this) to try to align as much as possible to the base defaults available. I don't think this would've gone as smoothly without your contribution and help. So thank you!

@BraisGabin BraisGabin deleted the detekt branch March 15, 2022 08:08
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants