Check if ktlint_code_style is set in .editorconfig before overriding it #2143

OlivierGenez · 2024-05-28T00:56:47Z

ktlint_code_style gets unconditionally overridden to its default value (intellij_idea) when the editorConfigOverride map is non-empty but does not define it. As a result, it gets overridden even if it's actually defined in the .editorconfig file.

This change checks if ktlint_code_style is already defined in .editorconfig before overriding it to its default value.

Fixes #2142.

ktlint_code_style gets unconditionally overridden to its default value (intellij_idea) when the editorConfigOverride map is non-empty but does not define it. As a result, it gets overridden even if it's actually defined in the .editorconfig file. This change checks if ktlint_code_style is already defined in .editorconfig before overriding it to its default value. Fixes diffplug#2142.

Goooler · 2024-05-28T02:59:08Z

I see it! Could you add a test case for this? Or just update

spotless/plugin-gradle/src/test/java/com/diffplug/gradle/spotless/KotlinExtensionTest.java

Lines 130 to 147 in 5f840aa

    
           @Test 
        
           void testSetEditorConfigCanOverrideEditorConfigFile() throws IOException { 
        
           	setFile(".editorconfig").toResource("kotlin/ktlint/intellij_idea/.editorconfig"); 
        
           	setFile("build.gradle").toLines( 
        
           			"plugins {", 
        
           			"    id 'org.jetbrains.kotlin.jvm' version '1.6.21'", 
        
           			"    id 'com.diffplug.spotless'", 
        
           			"}", 
        
           			"repositories { mavenCentral() }", 
        
           			"spotless {", 
        
           			"    kotlin {", 
        
           			"        ktlint().editorConfigOverride([", 
        
           			"            ktlint_code_style: \"ktlint_official\",", 
        
           			"        ])", 
        
           			"    }", 
        
           			"}"); 
        
           	checkKtlintOfficialStyle(); 
        
           }

OlivierGenez · 2024-05-28T03:41:03Z

I see it! Could you add a test case for this? Or just update

spotless/plugin-gradle/src/test/java/com/diffplug/gradle/spotless/KotlinExtensionTest.java

Lines 130 to 147 in 5f840aa

@Test

void testSetEditorConfigCanOverrideEditorConfigFile() throws IOException {

setFile(".editorconfig").toResource("kotlin/ktlint/intellij_idea/.editorconfig");

setFile("build.gradle").toLines(

"plugins {",

" id 'org.jetbrains.kotlin.jvm' version '1.6.21'",

" id 'com.diffplug.spotless'",

"}",

"repositories { mavenCentral() }",

"spotless {",

" kotlin {",

" ktlint().editorConfigOverride([",

" ktlint_code_style: \"ktlint_official\",",

" ])",

" }",

"}");

checkKtlintOfficialStyle();

}

Thanks for the quick reply!

The PR already has this test: https://github.com/diffplug/spotless/pull/2143/files#diff-7bbe43d03149d8aad5b95564d57dc5bf59cba4ce539a44e01bbe51215dcdae71R131-R147. Is it sufficient, or is there a specific/additional scenario I am missing that you're after?

Goooler · 2024-05-28T03:43:46Z

Ooooops, will take a look later.

Goooler · 2024-05-29T02:50:57Z

...int1Dot0Dot0/java/com/diffplug/spotless/glue/ktlint/compat/KtLintCompat1Dot0Dot0Adapter.java

-		if (!editorConfigOverrideMap.containsKey("ktlint_code_style")) {
+		boolean isCodeStyleDefinedInEditorConfig = editorConfig.getValue().getSections().stream()
+				.anyMatch(section -> section.getProperties().containsKey("ktlint_code_style"));
+		if (!isCodeStyleDefinedInEditorConfig && !editorConfigOverrideMap.containsKey("ktlint_code_style")) {


The key point is here, I missed checking EditorConfig file properties in #1808.

Goooler · 2024-05-29T02:53:17Z

plugin-gradle/src/test/java/com/diffplug/gradle/spotless/KotlinExtensionTest.java

+				"spotless {",
+				"    kotlin {",
+				"        ktlint().editorConfigOverride([",
+				"	         ktlint_test_key: true,",


Nice, can you add a test case like 87537c0 for maven as well?

Done!

While adding the test I realised that plugin-maven never defaulted EditorConfig's path to .editorconfig, so I fixed that too.

The default path was never set, which means that .editorconfig was never picked up if not explicitly set.

This verifies that the previous fix which checks if ktlint_code_style is already defined in .editorconfig before overriding it works as expected in plugin-maven.

CHANGES.md

plugin-gradle/CHANGES.md

plugin-maven/CHANGES.md

Goooler · 2024-05-31T04:15:24Z

plugin-maven/src/main/java/com/diffplug/spotless/maven/kotlin/Ktlint.java

+		if (editorConfigPath == null) {
+			File defaultEditorConfig = new File(".editorconfig");
+			if (defaultEditorConfig.exists()) {
+				editorConfigPath = defaultEditorConfig.getPath();
+			}
+		}


Will load EMPTY_EDITOR_CONFIG_DEFAULTS before

spotless/lib/src/compatKtLint1Dot0Dot0/java/com/diffplug/spotless/glue/ktlint/compat/KtLintCompat1Dot0Dot0Adapter.java

Lines 96 to 101 in cf6c76f

EditorConfigDefaults editorConfig;

if (editorConfigPath == null || !Files.exists(editorConfigPath)) {

editorConfig = EditorConfigDefaults.Companion.getEMPTY_EDITOR_CONFIG_DEFAULTS();

} else {

editorConfig = EditorConfigDefaults.Companion.load(editorConfigPath, RuleProviderKt.propertyTypes(allRuleProviders));

}

But this should parse the root .editorconfig, I forget the exact position, let me check.

You are right! KtLintRuleEngine will load the .editorconfig.

However I believe we still need to load it ourselves as well in order to check if it sets ktlint_code_style in KtLintCompat1Dot0Dot0Adapter. Otherwise we will run into the original issue: ktlint_code_style will be overridden to its default value (intellij_idea) when the editorConfigOverride map is non-empty but does not define it. This is also what plugin-gradle does:

spotless/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/BaseKotlinExtension.java

Lines 159 to 160 in add4c1e

File defaultEditorConfig = getProject().getRootProject().file(".editorconfig");

FileSignature editorConfigPath = defaultEditorConfig.exists() ? FileSignature.signAsList(defaultEditorConfig) : null;

Co-authored-by: Zongle Wang <wangzongler@gmail.com>

Goooler · 2024-06-03T02:50:17Z

plugin-maven/src/main/java/com/diffplug/spotless/maven/kotlin/Ktlint.java

+		if (editorConfigPath == null) {
+			File defaultEditorConfig = new File(".editorconfig");
+			if (defaultEditorConfig.exists()) {
+				editorConfigPath = defaultEditorConfig.getPath();
+			}
+		}


Suggested change

if (editorConfigPath == null) {

File defaultEditorConfig = new File(".editorconfig");

if (defaultEditorConfig.exists()) {

editorConfigPath = defaultEditorConfig.getPath();

}

}

if (editorConfigPath == null && defaultEditorConfig.exists()) {

editorConfigPath = defaultEditorConfig.getPath();

}

And add

private static final File defaultEditorConfig = new File(".editorconfig");

Quick shot!

This comment was marked as outdated.

Sign in to view

Goooler approved these changes May 29, 2024

View reviewed changes

OlivierGenez added 3 commits May 29, 2024 22:36

Default EditorConfig path to ".editorconfig" in plugin-maven ktlint step

0be12f5

The default path was never set, which means that .editorconfig was never picked up if not explicitly set.

Add ktlint test to plugin-maven

ccf141e

This verifies that the previous fix which checks if ktlint_code_style is already defined in .editorconfig before overriding it works as expected in plugin-maven.

Update CHANGES.md files

1453214

Goooler reviewed May 31, 2024

View reviewed changes

OlivierGenez and others added 2 commits May 31, 2024 23:11

Fix CHANGES.md files links

2effcbd

Co-authored-by: Zongle Wang <wangzongler@gmail.com>

Revert unneeded changes to KtlintTest

f33f8be

Goooler approved these changes Jun 3, 2024

View reviewed changes

Apply minor Ktlint implementation refactor

397a7fd

nedtwigg merged commit 3308d33 into diffplug:main Jun 4, 2024
14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check if ktlint_code_style is set in .editorconfig before overriding it #2143

Check if ktlint_code_style is set in .editorconfig before overriding it #2143

OlivierGenez commented May 28, 2024 •

edited

Loading

This comment was marked as outdated.

Goooler commented May 28, 2024 •

edited

Loading

OlivierGenez commented May 28, 2024

Goooler commented May 28, 2024

Goooler May 29, 2024 •

edited

Loading

Goooler May 29, 2024

OlivierGenez May 29, 2024

Goooler May 31, 2024

OlivierGenez Jun 1, 2024

Goooler Jun 3, 2024

Goooler Jun 3, 2024

OlivierGenez Jun 3, 2024

Goooler Jun 3, 2024

	EditorConfigDefaults editorConfig;
	if (editorConfigPath == null \|\| !Files.exists(editorConfigPath)) {
	editorConfig = EditorConfigDefaults.Companion.getEMPTY_EDITOR_CONFIG_DEFAULTS();
	} else {
	editorConfig = EditorConfigDefaults.Companion.load(editorConfigPath, RuleProviderKt.propertyTypes(allRuleProviders));
	}

	File defaultEditorConfig = getProject().getRootProject().file(".editorconfig");
	FileSignature editorConfigPath = defaultEditorConfig.exists() ? FileSignature.signAsList(defaultEditorConfig) : null;

Check if ktlint_code_style is set in .editorconfig before overriding it #2143

Check if ktlint_code_style is set in .editorconfig before overriding it #2143

Conversation

OlivierGenez commented May 28, 2024 • edited Loading

This comment was marked as outdated.

Goooler commented May 28, 2024 • edited Loading

OlivierGenez commented May 28, 2024

Goooler commented May 28, 2024

Goooler May 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OlivierGenez commented May 28, 2024 •

edited

Loading

Goooler commented May 28, 2024 •

edited

Loading

Goooler May 29, 2024 •

edited

Loading