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

Default Gradle config path should be config/detekt/config.yml #1262

Closed
3flex opened this issue Oct 19, 2018 · 10 comments · Fixed by #1801
Closed

Default Gradle config path should be config/detekt/config.yml #1262

3flex opened this issue Oct 19, 2018 · 10 comments · Fixed by #1801

Comments

@3flex
Copy link
Member

3flex commented Oct 19, 2018

Expected Behavior

Default config path is one of:

  • $rootProject.projectDir/config/detekt/config.yml
  • $projectDir/config/detekt/config.yml

Current Behavior

Config is generated at $projectDir/default-detekt-config.yml

Context

Sensible defaults make for a better onboarding experience. Gradle conventions could be followed to generate the config file at a conventional path instead of in the project root.

Placing the file in $projectDir/config/detekt/config.yml makes most sense, so a different config is used for each Gradle project (important for example when there are Android-specific rules that apply to one project that don't apply to other projects in a multi-project build).

@3flex 3flex changed the title Default config path is config/detekt/config.yml Default config path should be config/detekt/config.yml Oct 19, 2018
@3flex 3flex changed the title Default config path should be config/detekt/config.yml Default Gradle config path should be config/detekt/config.yml Oct 19, 2018
@BorzdeG
Copy link

BorzdeG commented Oct 19, 2018

It may be possible to change the search order of the configuration file to:

  • $projectDir/detekt.yml
  • $projectDir/config/detekt/config.yml
  • $rootDir/detekt.yml
  • $rootDir/config/detekt/config.yml

@arturbosch
Copy link
Member

+1, a search order like @BorzdeG mentioned soulds good to me

@3flex
Copy link
Member Author

3flex commented Oct 22, 2018

If the default is made up of multiple paths, then where would the file be created when detektGenerateConfig task is run?

It should be $rootDir/config/detekt/config.yml (or $projectDir/config/detekt/config.yml) and that should also be the first location checked in the search order.

Also will the search order just match on the first located config, or will it apply each found file to the --config parameter in the CLI?

I'm not a big fan of the search order idea to be honest... I think it's better to have a single path that is the default for a Gradle project, and if the user wants to move it elsewhere, they can configure themselves. Don't want too much magic or complexity in the Gradle plugin. The only question is should a default be set that applies to the entire project, or should config be per subproject?

@arturbosch
Copy link
Member

Hm, the idea of the config and baseline was that there is one source of truth. Now with the more idiomatic gradle plugin we generate one baseline and config per module (detektBaseline & detekteCreateConfig). We haven't thought much of this change to be honest, so thank you for bringing this up.

Actually I would revert this change so these two tasks would only be possible on the root project. More than one config must be provided via property. Having a search order can confuse not only the user but also us, developers. I would agree with @3flex here.

What do you think?

@3flex
Copy link
Member Author

3flex commented Oct 24, 2018

Agree with @arturbosch that a single config applied to all subprojects by default makes sense, so default path should be $rootDir/config/detekt/config.yml and not per-module/subproject - if user wants a different config in subprojects they can set that up themselves.

I'm less sure about detektCreateConfig behaviour. Definitely if user has not overridden the path for the config file, then detektCreateConfig should create a default config in the root project config folder and nowhere else. If a file already exists in that path it should not be overwritten.

It gets more complicated with subprojects and root project if user sets detekt.config value in multiple build.gradle files and expects to be able to run detektCreateConfig. I think maybe detektCreateConfig should only work if the path is $rootDir/config/detekt/config.yml, but if user wants to create additional config files either in root or subprojects, they have to do that manually? Can provide appropriate documentation.

This offers nice defaults and a way to generate the config so it can be tweaked. Anyone who wants a more complex setup will have to setup themselves. I think that's reasonable.

@BorzdeG
Copy link

BorzdeG commented Oct 24, 2018

I agree with my colleagues - $rootDir/config/detekt/config.yml is enough for the default path. For the rest, there are manual settings.

@arturbosch
Copy link
Member

Agree with @3flex , the default config is always generated on the default path and this is also always displayed on the console. This task should be independant of the config property or whatever. Its not so hard to move the file, so I would prefer a hard default :).

@marschwar
Copy link
Contributor

Maybe as an interesting side note: https://docs.gradle.org/5.0-rc-1/release-notes.html#potential-breaking-changes

Checkstyle plugin config directory in multi-project builds
Gradle will now, by convention, only look for Checkstyle configuration files in the root project's config/checkstyle directory. Checkstyle configuration files in subprojects — the old by-convention location — will be ignored unless you explicitly configure their path via checkstyle.configDir or checkstyle.config.

@3flex
Copy link
Member Author

3flex commented Jul 28, 2019

@arturbosch request adding this to 1.0.0 milestone. Not sure when you plan to release but it would be best to do this before first major release.

I plan a PR in the next day or two.

@arturbosch
Copy link
Member

@3flex alright, looking forward to this!

@3flex 3flex added this to the 1.0.0 milestone Jul 30, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Nov 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants