fix: file end of line character changes (#197)

* fix: file end of line character changes

Maintain the current predominant end of line character for files when
making edits. This changes editing to use the standard TextEdit insert
and replace creation methods to reduce complexity and overhead.

This only handles Unix '\n' and Windows '\r\n' line endings, not pre
MacOSX Mac '\r' as CodeNar doesn't appear to support that line ending.

Fix callback handling in tests, adding missing await and reject
handling.

Validate each file result in folder lint test instead of total.

Use option chaining and early return in getUpdatedSource to improve
readability.

Remove use of deprecated isNullOrUndefined.

Fixes #176

* ci: disable mega linter commits to forks

Disable mega linter generation of commits to repo forks as this will
generally fail.

.gitattributes

@@ -0,0 +1,3 @@
+client/src/test/examples/tiny-lf.groovy text eol=lf
+client/src/test/examples/tiny-cr.groovy text eol=cr
+client/src/test/examples/tiny-crlf.groovy text eol=crlf

.github/workflows/mega-linter.yml

@@ -80,10 +80,10 @@ jobs:
 
       # Push new commit if applicable
       - name: Prepare commit
-        if: steps.changes.outputs.changed == 1 && (env.APPLY_FIXES_EVENT == 'all' || env.APPLY_FIXES_EVENT == github.event_name) && env.APPLY_FIXES_MODE == 'commit' && github.ref != 'refs/heads/master'
+        if: steps.changes.outputs.changed == 1 && (env.APPLY_FIXES_EVENT == 'all' || env.APPLY_FIXES_EVENT == github.event_name) && env.APPLY_FIXES_MODE == 'commit' && github.ref != 'refs/heads/master' && github.repository == github.event.pull_request.head.repo.full_name
         run: sudo chown -Rc $UID .git/
       - name: Commit and push applied linter fixes
-        if: steps.changes.outputs.changed == 1 && (env.APPLY_FIXES_EVENT == 'all' || env.APPLY_FIXES_EVENT == github.event_name) && env.APPLY_FIXES_MODE == 'commit' && github.ref != 'refs/heads/master'
+        if: steps.changes.outputs.changed == 1 && (env.APPLY_FIXES_EVENT == 'all' || env.APPLY_FIXES_EVENT == github.event_name) && env.APPLY_FIXES_MODE == 'commit' && github.ref != 'refs/heads/master' && github.repository == github.event.pull_request.head.repo.full_name
         uses: stefanzweifel/git-auto-commit-action@v5
         with:
           branch: ${{ github.event.pull_request.head.ref || github.head_ref || github.ref }}

CHANGELOG.md

@@ -2,6 +2,8 @@
 
 ## [Unreleased]
 
+- fix: file end of line character changes ([#176](https://github.com/nvuillam/vscode-groovy-lint/issues/176))
+
 ## [3.1.0] 2023-12-17
 
 - Added new setting `showProblemsView` that controls if Problems View should open after initial lint pass

client/src/test/examples/tiny-lf.groovy

@@ -0,0 +1,56 @@
+import groovy.io.FileType
+import groovy.json.*
+import groovy.time.TimeCategory
+import static groovyx.gpars.GParsPool.withPool
+
+        def script = new GroovyScriptEngine( "." ).with{
+                loadScriptByName( 'Utils.groovy' ) ;
+   }
+this.metaClass.mixin script
+
+def returnCode = 0
+         Exception eThrow = null ;
+   try {
+    initialize(args) ;
+} catch (Exception e){
+                 eThrow = e ;
+    returnCode = 1
+             }
+          if (eThrow == null){
+                      return 0 ;
+}
+else {
+         throw eThrow ;
+    return 1 ;
+}
+
+//////////////////////////////////////////////////////////////////////////////////////////////////////////
+///////////////////////////////////////////////// SCRIPT /////////////////////////////////////////////////
+//////////////////////////////////////////////////////////////////////////////////////////////////////////
+def initialize(args3) {   //
+    def executor = new TestExecutor(args3)
+    return executor
+}
+
+class TestExecutor {
+
+    public TestExecutor( args2) {
+        this.testExternalGlobalProps()
+    }
+
+    public testExternalGlobalProps() {
+        Utils.printlnLog( '########## testExternalGlobalProps')
+        def globalKeyName = new Random().with { (1..9).collect { (('a'..'z')).join()[ nextInt((('a'..'z')).join().length())]}.join()}
+        Utils.printlnLog( "Generated random key: ${globalKeyName}")
+        Utils.setExternalValue(globalKeyName , 'lelama' , 'nul')
+        def storedValue = Utils.getExternalValue(globalKeyName , 'lelama')
+        assert storedValue == 'nul' , 'Error in global prop key storage/ retrieval (1)'
+        Utils.setExternalValue(globalKeyName , 'lelama2' , 'nul2')
+        def storedValue2 = Utils.getExternalValue(globalKeyName , 'lelama2')
+        assert storedValue2 == 'nul2' , 'Error in global prop key storage/ retrieval (2)'
+        def storedValueBack = Utils.getExternalValue(globalKeyName , 'lelama')
+        assert storedValueBack == 'nul' , 'Error in global prop key storage/ retrieval (3)'
+        Utils.printlnLog( Utils.getExternalValue(globalKeyName))
+    }
+
+}

client/src/test/suite/extension.test.ts

@@ -21,14 +21,21 @@ const testsFolder = '../../../src/test';
 const examples = 'examples';
 const testConfig = '.groovylintrc.json';
 const extensionId = 'NicolasVuillamy.vscode-groovy-lint';
-const tinyGroovy = 'tiny.groovy';
+const tinyGroovy = 'tiny-crlf.groovy';
 const validGroovy = 'valid.groovy';
 const numberOfGroovyLintCommands = 9;
 
 // Additional timeout for the first test to
 // allow for server startup timeout.
 let additionalTimeout = defaultTimeout;
 
+// RegExp to determine predominate End Of Line (EOL) sequence.
+const eolCaptureRegExp: RegExp = new RegExp(/(\r?\n)/g);
+
+// End of Line sequences.
+const unixEOL = '\n';
+const dosEOL = '\r\n';
+
 /**
  * testDocumentDetails represents the expected results for a testDocument.
  */
@@ -54,6 +61,7 @@ const documentDetails = new Map<string, testDocumentDetails>();
 [
 	new testDocumentDetails(validGroovy, 0, 0, false),
 	new testDocumentDetails(tinyGroovy, 50, 19),
+	new testDocumentDetails('tiny-lf.groovy', 50, 19),
 	new testDocumentDetails('big.groovy', 4114, 789, true, 10 * second),
 	new testDocumentDetails('Jenkinsfile', 380, 151, true, 10 * second),
 	new testDocumentDetails('parseError.groovy', 2, 1, false),
@@ -428,15 +436,19 @@ class testDocuments extends Map<string, testDocument> {
 				this.forEach(doc => doc.reject());
 			}, timeout);
 
-			// Close stale editors.
-			await executeCommand('workbench.action.closeAllEditors');
+			try {
+				// Close stale editors.
+				await executeCommand('workbench.action.closeAllEditors');
 
-			doc = await doc.setupTest();
-			if (action) {
-				action(doc, this);
-			}
+				doc = await doc.setupTest();
+				if (action) {
+					await action(doc, this);
+				}
 
-			resolve();
+				resolve();
+			} catch (err) {
+				reject(err);
+			}
 		}).finally(async () => {
 			clearTimeout(timer);
 			await this.cleanup();
@@ -503,8 +515,11 @@ suite('VsCode GroovyLint Test Suite', async function() {
 	// Format documents.
 	documentDetails.forEach(async (_, name) => {
 		test(`Format "${name}"`, async function() {
+			// Total process requires two passes so double the timeout.
+			const docDetails = documentDetails.get(name);
 			await testSingle(this, name, async function(doc: testDocument): Promise<void> {
 				const textBefore = doc.text;
+				const origEOL: string = textEOL(textBefore);
 				const textEdits = await executeCommand('vscode.executeFormatDocumentProvider', doc.uri, {});
 				if (!doc.formats) {
 					// Not expected for format.
@@ -513,10 +528,13 @@ suite('VsCode GroovyLint Test Suite', async function() {
 				}
 
 				await doc.applyEdits(textEdits);
-				const textAfter = doc.text;
 
+				const textAfter = doc.text;
 				assert(textBefore !== textAfter, 'Text not updated format');
-			});
+
+				const newEOL: string = textEOL(textAfter);
+				assert(origEOL === newEOL, `end of line sequence changed from ${origEOL} to ${newEOL}`);
+			}, docDetails.timeout * 2);
 		});
 	});
 
@@ -527,6 +545,7 @@ suite('VsCode GroovyLint Test Suite', async function() {
 			const docDetails = documentDetails.get(name);
 			await testSingle(this, name, async function(doc: testDocument): Promise<void> {
 				const textBefore = doc.text;
+				const origEOL: string = textEOL(textBefore);
 				const promiseNames: string[] = [];
 				if (doc.lint !== doc.lintFix) {
 					promiseNames.push(testDocument.diagnosticPromise, testDocument.documentEditPromise);
@@ -544,43 +563,65 @@ suite('VsCode GroovyLint Test Suite', async function() {
 					assert(textBefore === textAfter, 'Unexpected text update');
 				}
 				assert(diagsLen === doc.lintFix, `Found ${diagsLen} diagnostics expected ${doc.lintFix}`);
+
+				const newEOL: string = textEOL(textAfter);
+				assert(origEOL === newEOL, `end of line sequence changed from ${origEOL} to ${newEOL}`);
 			}, docDetails.timeout * 3);
 		});
 	});
 
 	// Disable rules for a line.
-	test('Disable next line', async function() {
-		await testSingle(this, tinyGroovy, async function(doc: testDocument): Promise<void> {
-			const lineNb = 5;
-			await doc.disableRule('groovyLint.disableRule', 'SpaceBeforeOpeningBrace', lineNb);
-			await doc.disableRule('groovyLint.disableRule', 'UnnecessaryGString', lineNb + 1); // Disable added a line.
+	documentDetails.forEach(async (_, name) => {
+		if (!name.startsWith('tiny')) {
+			return;
+		}
+
+		test(`Disable "${name}" next line`, async function() {
+			await testSingle(this, name, async function(doc: testDocument): Promise<void> {
+				const origEOL: string = textEOL(doc.text);
+				const lineNb = 5;
+				// Add a disable line.
+				await doc.disableRule('groovyLint.disableRule', 'SpaceBeforeOpeningBrace', lineNb);
 
-			const disableLine = doc.doc.lineAt(lineNb).text;
+				// Amend the disable line.
+				await doc.disableRule('groovyLint.disableRule', 'UnnecessaryGString', lineNb + 1);
 
-			assert(disableLine.includes(`/* groovylint-disable-next-line SpaceBeforeOpeningBrace, UnnecessaryGString */`),
-				`groovylint-disable-next-line not added correctly found: ${disableLine}`
-			);
+				const disableLine = doc.doc.lineAt(lineNb).text;
+				assert(disableLine.includes(`/* groovylint-disable-next-line SpaceBeforeOpeningBrace, UnnecessaryGString */`),
+					`groovylint-disable-next-line not added correctly found: ${disableLine}`
+				);
+
+				const newEOL: string = textEOL(doc.text);
+				assert(origEOL === newEOL, `end of line sequence changed from ${origEOL} to ${newEOL}`);
+			});
 		});
 	});
 
 	// Disable rules for entire file.
 	test('Disable rules in all file', async function() {
 		await testSingle(this, tinyGroovy, async function(doc: testDocument): Promise<void> {
+			const origEOL: string = textEOL(doc.text);
+			// Add a disable line.
 			await doc.disableRule('groovyLint.disableRuleInFile', 'CompileStatic');
+
+			// Amend the disable line.
 			await doc.disableRule('groovyLint.disableRuleInFile', 'DuplicateStringLiteral');
 
 			const disableLine = doc.doc.lineAt(0).text;
-
 			assert(disableLine.includes('/* groovylint-disable CompileStatic, DuplicateStringLiteral */'),
 				`groovylint-disable not added correctly found: ${disableLine}`
 			);
+
+			const newEOL: string = textEOL(doc.text);
+			assert(origEOL === newEOL, `end of line sequence changed from ${origEOL} to ${newEOL}`);
 		});
 	});
 
 	// Quick fix error.
 	test('Quick fix error in tiny document', async function() {
 		await testSingle(this, tinyGroovy, async function(doc: testDocument): Promise<void> {
 			const textBefore = doc.text;
+			const origEOL: string = textEOL(textBefore);
 			const diagnostic = doc.diags.find(diag => (diag.code as string).startsWith('UnnecessarySemicolon'));
 			// Request code actions.
 			await doc.execute([], 'vscode.executeCodeActionProvider', doc.uri, diagnostic.range);
@@ -590,37 +631,41 @@ suite('VsCode GroovyLint Test Suite', async function() {
 			const textAfter = doc.text;
 
 			assert(textBefore !== textAfter, 'Text not updated');
+
+			const newEOL: string = textEOL(textAfter);
+			assert(origEOL === newEOL, `end of line sequence changed from ${origEOL} to ${newEOL}`);
 		});
 	});
 
 	// Quick fix all error types.
 	test('Quick fix error in entire file in tiny document', async function() {
 		await testSingle(this, tinyGroovy, async function(doc: testDocument): Promise<void> {
 			const textBefore = doc.text;
+			const origEOL: string = textEOL(textBefore);
 			const diagnostic = doc.diags.find(diag => (diag.code as string).startsWith('UnnecessarySemicolon'));
 
 			// Request code actions.
 			await doc.execute([], 'vscode.executeCodeActionProvider', doc.uri, diagnostic.range);
 
 			// Apply Quick Fix.
 			await doc.execute([testDocument.diagnosticPromise, testDocument.documentEditPromise], 'groovyLint.quickFixFile', doc.uri.toString(), diagnostic);
-			const textAfter = doc.text;
 
+			const textAfter = doc.text;
 			assert(textBefore !== textAfter, 'Text not updated');
+
+			const newEOL: string = textEOL(textAfter);
+			assert(origEOL === newEOL, `end of line sequence changed from ${origEOL} to ${newEOL}`);
 		});
 	});
 
 	// Lint a folder.
 	test('Lint folder', async function() {
 		let timeout = 0;
-		let expected = 0;
 		documentDetails.forEach(doc => {
 			timeout += doc.timeout;
-			expected += doc.lint;
 		});
 		await testMulti(this, [...documentDetails.keys()], async function(doc: testDocument, testDocs: testDocuments): Promise<void> {
 			const promises: Promise<void>[] = [];
-			let expected = 0;
 			testDocs.forEach(doc => {
 				promises.push(doc.wait(testDocument.diagnosticPromise));
 			});
@@ -631,14 +676,9 @@ suite('VsCode GroovyLint Test Suite', async function() {
 
 			await Promise.all(promises);
 
-			let diags = 0;
 			testDocs.forEach(doc => {
-				diags += doc.diags.length;
+				assert(doc.lint === doc.diags.length, `"${doc.name}" has: ${doc.diags.length} diagnostics expected: ${doc.lint}`);
 			});
-
-			await testDocs.cleanup();
-
-			assert(diags === expected, `Found ${diags} diagnostics expected: ${expected}`);
 		}, timeout);
 	});
 
@@ -657,3 +697,27 @@ async function executeCommand(command: string, ...args: any[]): Promise<any> {
 	debug(`Execute command ${command} with args ${JSON.stringify(args)}`);
 	return vscode.commands.executeCommand(command, ...args);
 }
+
+/**
+ * Returns the name of the predominate End Of Line (EOL) of the given text.
+ *
+ * @param text the text to determine the EOL from.
+ * @returns the name of the predominate EOL sequence of text.
+ */
+function textEOL(text: string): string {
+	const eols: string[] = text.match(eolCaptureRegExp) || [];
+	let dos: number = 0;
+	let unix: number = 0;
+	eols.forEach(eol => {
+		switch (eol) {
+			case dosEOL:
+				dos++;
+				break;
+			case unixEOL:
+				unix++;
+				break;
+		}
+	});
+
+	return unix > dos ? unixEOL : dosEOL;
+}

server/src/DocumentsManager.ts

@@ -2,9 +2,8 @@ import { TextDocuments, Diagnostic, WorkspaceFolder } from 'vscode-languageserve
 import { TextDocument, DocumentUri, TextEdit } from 'vscode-languageserver-textdocument';
 import { executeLinter } from './linter';
 import { applyQuickFixes, applyQuickFixesInFile, disableErrorWithComment, disableErrorForProject } from './codeActions';
-import { isTest, showRuleDocumentation } from './clientUtils';
+import { isTest, showRuleDocumentation, eolAndLines } from './clientUtils';
 import { URI } from 'vscode-uri';
-import { EOL } from 'os';
 import { resolve } from 'path';
 import { StatusNotification, VsCodeGroovyLintSettings } from './types';
 import { lintFolder } from './folder';
@@ -377,16 +376,6 @@ export class DocumentsManager {
 		return this.documents.get(textDocument.uri) || textDocument; // Or expression, in case the textDocument is not opened yet
 	}
 
-	// Split source string into array of lines
-	getTextDocumentLines(textDocument: TextDocument): string[] {
-		// TODO(steve): Use TextDocument.eol instead of EOL so we
-		// maintain the original line endings.
-		let normalizedString = textDocument.getText() + "";
-		normalizedString = normalizedString.replace(/\r/g, "");
-		normalizedString = normalizedString.replace(/\n/g, EOL);
-		return normalizedString.split(EOL);
-	}
-
 	// Update diagnostics on client and store them in docsDiagnostics field
 	async updateDiagnostics(docUri: string, diagnostics: Diagnostic[]): Promise<void> {
 		debug(`Update diagnostics for ${docUri}: ${diagnostics.length} diagnostics sent`);