Fix #118 Consider context when suggesting variables

When using textDocument/completion on variables, the context of the current line should be considered. Instead of simply returning all the variables that have been defined in the Dockerfile, only the variables that have been declared before the current line should be suggested. Signed-off-by: Remy Suen <[email protected]>
rcjsuen · Aug 1, 2017 · 8328959 · 8328959
1 parent ed824f2
commit 8328959
Show file tree

Hide file tree

Showing 4 changed files with 45 additions and 10 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -8,6 +8,7 @@ All notable changes to this project will be documented in this file.
 
 ### Fixed
 - fixed parsing of escaped whitespace values in ENV instructions ([#115](https://github.com/rcjsuen/dockerfile-language-server-nodejs/issues/115))
+- prevent undeclared variables from being suggested as completion items ([#118](https://github.com/rcjsuen/dockerfile-language-server-nodejs/issues/118))
 
 ## [0.0.2] - 2017-07-31
 ### Added

diff --git a/src/dockerAssist.ts b/src/dockerAssist.ts
@@ -85,7 +85,7 @@ export class DockerAssist {
 					if (variablePrefix === "") {
 						// empty prefix, return all variables
 						let items = [];
-						for (let variable of dockerfile.getVariableNames()) {
+						for (let variable of dockerfile.getVariableNames(position.line)) {
 							let doc = dockerfile.getVariableValue(variable, position.line);
 							items.push(this.createVariableCompletionItem("${" + variable + '}', prefixLength, offset, true, doc));
 						}
@@ -97,7 +97,7 @@ export class DockerAssist {
 							variablePrefix = variablePrefix.substring(1);
 						}
 						let items = [];
-						for (let variable of dockerfile.getVariableNames()) {
+						for (let variable of dockerfile.getVariableNames(position.line)) {
 							if (variable.toLowerCase().indexOf(variablePrefix) === 0) {
 							let doc = dockerfile.getVariableValue(variable, position.line);
 								items.push(this.createVariableCompletionItem(brace ? "${" + variable + '}' : '$' + variable, prefixLength, offset, brace, doc));

diff --git a/src/parser/dockerfile.ts b/src/parser/dockerfile.ts
@@ -164,7 +164,7 @@ export class Dockerfile {
 		return this.directive;
 	}
 
-	public getVariableNames(): string[] {
+	public getVariableNames(currentLine: number): string[] {
 		let variables = [
 			"FTP_PROXY", "ftp_proxy",
 			"HTTP_PROXY", "http_proxy",
@@ -173,17 +173,21 @@ export class Dockerfile {
 		];
 
 		for (let arg of this.getARGs()) {
-			let variable = arg.getProperty().getName();
-			if (variables.indexOf(variable) === -1) {
-				variables.push(variable);
+			if (arg.isBefore(currentLine)) {
+				let variable = arg.getProperty().getName();
+				if (variables.indexOf(variable) === -1) {
+					variables.push(variable);
+				}
 			}
 		}
 
 		for (let env of this.getENVs()) {
-			for (let property of env.getProperties()) {
-				let variable = property.getName();
-				if (variables.indexOf(variable) === -1) {
-					variables.push(variable);
+			if (env.isBefore(currentLine)) {
+				for (let property of env.getProperties()) {
+					let variable = property.getName();
+					if (variables.indexOf(variable) === -1) {
+						variables.push(variable);
+					}
 				}
 			}
 		}

diff --git a/test/dockerAssist.test.ts b/test/dockerAssist.test.ts
@@ -1668,6 +1668,11 @@ describe('Docker Content Assist Tests', function() {
 				assertVariable("foo", items[1], 3, 9, 2, false, "bar");
 				assertVariable("FTP_PROXY", items[2], 3, 9, 2, false);
 				assertVariable("ftp_proxy", items[3], 3, 9, 2, false);
+
+				items = computePosition("FROM busybox\nRUN echo $f\nARG foo=bar\nARG FOO=BAR", 1, 11);
+				assert.equal(items.length, 2);
+				assertVariable("FTP_PROXY", items[0], 1, 9, 2, false);
+				assertVariable("ftp_proxy", items[1], 1, 9, 2, false);
 			});
 
 			it("ENV variable", function() {
@@ -1697,6 +1702,11 @@ describe('Docker Content Assist Tests', function() {
 				assertVariable("foo", items[1], 3, 9, 2, false, "bar");
 				assertVariable("FTP_PROXY", items[2], 3, 9, 2, false);
 				assertVariable("ftp_proxy", items[3], 3, 9, 2, false);
+
+				items = computePosition("FROM busybox\nRUN echo $f\nENV foo=bar\nENV FOO=BAR", 1, 11);
+				assert.equal(items.length, 2);
+				assertVariable("FTP_PROXY", items[0], 1, 9, 2, false);
+				assertVariable("ftp_proxy", items[1], 1, 9, 2, false);
 			});
 
 			it("ARG and ENV variable", function() {
@@ -1726,6 +1736,11 @@ describe('Docker Content Assist Tests', function() {
 				assertVariable("foo", items[1], 4, 9, 2, false, "env");
 				assertVariable("FTP_PROXY", items[2], 4, 9, 2, false);
 				assertVariable("ftp_proxy", items[3], 4, 9, 2, false);
+
+				items = computePosition("FROM busybox\nRUN echo $f\nARG foo=arg\nARG FOO=ARG\nENV foo=env FOO=ENV", 1, 11);
+				assert.equal(items.length, 2);
+				assertVariable("FTP_PROXY", items[0], 1, 9, 2, false);
+				assertVariable("ftp_proxy", items[1], 1, 9, 2, false);
 			});
 
 			it("escaped", function() {
@@ -1766,6 +1781,11 @@ describe('Docker Content Assist Tests', function() {
 				assertVariable("foo", items[1], 3, 9, 3, true, "bar");
 				assertVariable("FTP_PROXY", items[2], 3, 9, 3, true);
 				assertVariable("ftp_proxy", items[3], 3, 9, 3, true);
+
+				items = computePosition("FROM busybox\nRUN echo ${f\nARG foo=bar\nARG FOO=BAR", 1, 12);
+				assert.equal(items.length, 2);
+				assertVariable("FTP_PROXY", items[0], 1, 9, 3, true);
+				assertVariable("ftp_proxy", items[1], 1, 9, 3, true);
 			});
 
 			it("ENV variable", function() {
@@ -1782,6 +1802,11 @@ describe('Docker Content Assist Tests', function() {
 				assertVariable("foo", items[1], 2, 9, 3, true, "bar");
 				assertVariable("FTP_PROXY", items[2], 2, 9, 3, true);
 				assertVariable("ftp_proxy", items[3], 2, 9, 3, true);
+
+				items = computePosition("FROM busybox\nRUN echo ${f\nENV foo=bar\nENV FOO=BAR", 1, 12);
+				assert.equal(items.length, 2);
+				assertVariable("FTP_PROXY", items[0], 1, 9, 3, true);
+				assertVariable("ftp_proxy", items[1], 1, 9, 3, true);
 			});
 
 			it("ARG and ENV variable", function() {
@@ -1798,6 +1823,11 @@ describe('Docker Content Assist Tests', function() {
 				assertVariable("foo", items[1], 4, 9, 3, true, "env");
 				assertVariable("FTP_PROXY", items[2], 4, 9, 3, true);
 				assertVariable("ftp_proxy", items[3], 4, 9, 3, true);
+
+				items = computePosition("FROM busybox\nRUN echo ${f\nARG foo=arg\nARG FOO=ARG\nENV foo=env FOO=ENV", 1, 12);
+				assert.equal(items.length, 2);
+				assertVariable("FTP_PROXY", items[0], 1, 9, 3, true);
+				assertVariable("ftp_proxy", items[1], 1, 9, 3, true);
 			});
 
 			it("escaped", function() {