Skip to content

Commit

Permalink
Fix #186 Correct range for unknown flag diagnostics
Browse files Browse the repository at this point in the history 
If the validator only uses the flag's name's range for the published
diagnostic, the range may potentially be of length zero if the flag
doesn't have a name if it simply the string "--". The handling and
rendering of diagnostics with zero length ranges is ambiguous and may
differ between clients so the diagnostic has been changed to instead
include the "--" prefix in its range so that its length is always at
least two.

Signed-off-by: Remy Suen <[email protected]>
  • Loading branch information
@rcjsuen
rcjsuen committed Oct 15, 2017
1 parent a7ff955 commit b4634f4
Show file tree
Hide file tree
Showing 5 changed files with 65 additions and 40 deletions.
4 changes: 4 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
# Changelog
All notable changes to this project will be documented in this file.

## [Unreleased]
### Fixed
- use a reasonable range for the diagnostic if an unknown flag has no name ([#186](https://github.com/rcjsuen/dockerfile-language-server-nodejs/issues/186))

## [0.0.9] - 2017-10-14
### Added
- settings
Expand Down
10 changes: 5 additions & 5 deletions src/dockerCommands.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,7 @@ export class DockerCommands {
uri
]:
[
TextEdit.replace(range, "interval")
TextEdit.replace(range, "--interval")
]
}
};
Expand All @@ -198,7 +198,7 @@ export class DockerCommands {
uri
]:
[
TextEdit.replace(range, "retries")
TextEdit.replace(range, "--retries")
]
}
};
Expand All @@ -209,7 +209,7 @@ export class DockerCommands {
uri
]:
[
TextEdit.replace(range, "start-period")
TextEdit.replace(range, "--start-period")
]
}
};
Expand All @@ -220,7 +220,7 @@ export class DockerCommands {
uri
]:
[
TextEdit.replace(range, "timeout")
TextEdit.replace(range, "--timeout")
]
}
};
Expand All @@ -231,7 +231,7 @@ export class DockerCommands {
uri
]:
[
TextEdit.replace(range, "from")
TextEdit.replace(range, "--from")
]
}
};
Expand Down
18 changes: 12 additions & 6 deletions src/dockerValidator.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -421,8 +421,8 @@ export class Validator {
for (const flag of healthcheckFlags) {
const flagName = flag.getName();
if (validFlags.indexOf(flagName) === -1) {
const nameRange = flag.getNameRange();
problems.push(Validator.createUnknownHealthcheckFlag(nameRange.start, nameRange.end, flag.getName()));
const range = flag.getRange();
problems.push(Validator.createUnknownHealthcheckFlag(range.start, flagName === "" ? range.end : flag.getNameRange().end, flag.getName()));
} else if (flagName === "retries") {
const value = flag.getValue();
if (value) {
Expand Down Expand Up @@ -518,9 +518,12 @@ export class Validator {
const addFlags = (instruction as ModifiableInstruction).getFlags();
for (let flag of addFlags) {
const name = flag.getName();
if (name !== "chown") {
const flagRange = flag.getRange();
if (name === "") {
problems.push(Validator.createUnknownAddFlag(flagRange.start, flagRange.end, name));
} else if (name !== "chown") {
let range = flag.getNameRange();
problems.push(Validator.createUnknownAddFlag(range.start, range.end, name));
problems.push(Validator.createUnknownAddFlag(flagRange.start, range.end, name));
}
}
this.checkFlagValue(addFlags, [ "chown" ], problems);
Expand All @@ -532,9 +535,12 @@ export class Validator {
if (flags.length > 0) {
for (let flag of flags) {
const name = flag.getName();
if (name !== "from" && name !== "chown") {
const flagRange = flag.getRange();
if (name === "") {
problems.push(Validator.createUnknownCopyFlag(flagRange.start, flagRange.end, name));
} else if (name !== "from" && name !== "chown") {
let range = flag.getNameRange();
problems.push(Validator.createUnknownCopyFlag(range.start, range.end, name));
problems.push(Validator.createUnknownCopyFlag(flagRange.start, range.end, name));
}
}
}
Expand Down
10 changes: 5 additions & 5 deletions test/dockerCommands.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -270,7 +270,7 @@ describe("Dockerfile execute commands", function () {
assert.equal(edit.documentChanges, undefined);
let edits = edit.changes[uri];
assert.equal(edits.length, 1);
assert.equal(edits[0].newText, "interval");
assert.equal(edits[0].newText, "--interval");
assert.equal(edits[0].range, range);
});

Expand All @@ -284,7 +284,7 @@ describe("Dockerfile execute commands", function () {
assert.equal(edit.documentChanges, undefined);
let edits = edit.changes[uri];
assert.equal(edits.length, 1);
assert.equal(edits[0].newText, "retries");
assert.equal(edits[0].newText, "--retries");
assert.equal(edits[0].range, range);
});

Expand All @@ -298,7 +298,7 @@ describe("Dockerfile execute commands", function () {
assert.equal(edit.documentChanges, undefined);
let edits = edit.changes[uri];
assert.equal(edits.length, 1);
assert.equal(edits[0].newText, "start-period");
assert.equal(edits[0].newText, "--start-period");
assert.equal(edits[0].range, range);
});

Expand All @@ -312,7 +312,7 @@ describe("Dockerfile execute commands", function () {
assert.equal(edit.documentChanges, undefined);
let edits = edit.changes[uri];
assert.equal(edits.length, 1);
assert.equal(edits[0].newText, "timeout");
assert.equal(edits[0].newText, "--timeout");
assert.equal(edits[0].range, range);
});

Expand All @@ -326,7 +326,7 @@ describe("Dockerfile execute commands", function () {
assert.equal(edit.documentChanges, undefined);
let edits = edit.changes[uri];
assert.equal(edits.length, 1);
assert.equal(edits[0].newText, "from");
assert.equal(edits[0].newText, "--from");
assert.equal(edits[0].range, range);
});
});
63 changes: 39 additions & 24 deletions test/dockerValidator.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1539,30 +1539,35 @@ describe("Docker Validator Tests", function() {
it("unknown flag", function() {
let diagnostics = validate("FROM alpine\nADD --x=bb . .");
assert.equal(diagnostics.length, 1);
assertUnknownAddFlag(diagnostics[0], "x", 1, 6, 1, 7);
assertUnknownAddFlag(diagnostics[0], "x", 1, 4, 1, 7);

diagnostics = validate("FROM alpine\nADD --chown=bb --x . .");
assert.equal(diagnostics.length, 1);
assertUnknownAddFlag(diagnostics[0], "x", 1, 17, 1, 18);
assertUnknownAddFlag(diagnostics[0], "x", 1, 15, 1, 18);

diagnostics = validate("FROM alpine\nADD --x --chown=bb . .");
assert.equal(diagnostics.length, 1);
assertUnknownAddFlag(diagnostics[0], "x", 1, 6, 1, 7);
assertUnknownAddFlag(diagnostics[0], "x", 1, 4, 1, 7);

// empty name
diagnostics = validate("FROM alpine\nADD -- . .");
assert.equal(diagnostics.length, 1);
assertUnknownAddFlag(diagnostics[0], "", 1, 4, 1, 6);

// empty value
diagnostics = validate("FROM alpine\nADD --x= . .");
assert.equal(diagnostics.length, 1);
assertUnknownAddFlag(diagnostics[0], "x", 1, 6, 1, 7);
assertUnknownAddFlag(diagnostics[0], "x", 1, 4, 1, 7);

// no equals sign
diagnostics = validate("FROM alpine\nADD --x . .");
assert.equal(diagnostics.length, 1);
assertUnknownAddFlag(diagnostics[0], "x", 1, 6, 1, 7);
assertUnknownAddFlag(diagnostics[0], "x", 1, 4, 1, 7);

// flags are case-sensitive
diagnostics = validate("FROM alpine\nADD --CHOWN=bb . .");
assert.equal(diagnostics.length, 1);
assertUnknownAddFlag(diagnostics[0], "CHOWN", 1, 6, 1, 11);
assertUnknownAddFlag(diagnostics[0], "CHOWN", 1, 4, 1, 11);
});

it("flag no value", function() {
Expand Down Expand Up @@ -1680,34 +1685,39 @@ describe("Docker Validator Tests", function() {
it("unknown flag", function() {
let diagnostics = validate("FROM alpine\nFROM busybox AS bb\nCOPY --x=bb . .");
assert.equal(diagnostics.length, 1);
assertUnknownCopyFlag(diagnostics[0], "x", 2, 7, 2, 8);
assertUnknownCopyFlag(diagnostics[0], "x", 2, 5, 2, 8);

diagnostics = validate("FROM alpine\nFROM busybox AS bb\nCOPY --from=bb --x . .");
assert.equal(diagnostics.length, 1);
assertUnknownCopyFlag(diagnostics[0], "x", 2, 17, 2, 18);
assertUnknownCopyFlag(diagnostics[0], "x", 2, 15, 2, 18);

diagnostics = validate("FROM alpine\nFROM busybox AS bb\nCOPY --x --from=bb . .");
assert.equal(diagnostics.length, 1);
assertUnknownCopyFlag(diagnostics[0], "x", 2, 7, 2, 8);
assertUnknownCopyFlag(diagnostics[0], "x", 2, 5, 2, 8);

// empty name
diagnostics = validate("FROM alpine\nFROM busybox AS bb\nCOPY -- . .");
assert.equal(diagnostics.length, 1);
assertUnknownCopyFlag(diagnostics[0], "", 2, 5, 2, 7);

// empty value
diagnostics = validate("FROM alpine\nFROM busybox AS bb\nCOPY --x= . .");
assert.equal(diagnostics.length, 1);
assertUnknownCopyFlag(diagnostics[0], "x", 2, 7, 2, 8);
assertUnknownCopyFlag(diagnostics[0], "x", 2, 5, 2, 8);

// no equals sign
diagnostics = validate("FROM alpine\nFROM busybox AS bb\nCOPY --x . .");
assert.equal(diagnostics.length, 1);
assertUnknownCopyFlag(diagnostics[0], "x", 2, 7, 2, 8);
assertUnknownCopyFlag(diagnostics[0], "x", 2, 5, 2, 8);

// flags are case-sensitive
diagnostics = validate("FROM alpine\nFROM busybox AS bb\nCOPY --FROM=bb . .");
assert.equal(diagnostics.length, 1);
assertUnknownCopyFlag(diagnostics[0], "FROM", 2, 7, 2, 11);
assertUnknownCopyFlag(diagnostics[0], "FROM", 2, 5, 2, 11);

diagnostics = validate("FROM alpine\nFROM busybox AS bb\nCOPY --CHOWN=bb . .");
assert.equal(diagnostics.length, 1);
assertUnknownCopyFlag(diagnostics[0], "CHOWN", 2, 7, 2, 12);
assertUnknownCopyFlag(diagnostics[0], "CHOWN", 2, 5, 2, 12);
});

it("flag no value", function() {
Expand Down Expand Up @@ -2191,34 +2201,39 @@ describe("Docker Validator Tests", function() {
it("unknown flag", function() {
let diagnostics = validate("FROM alpine\nHEALTHCHECK --interva=30s CMD ls");
assert.equal(diagnostics.length, 1);
assertUnknownHealthcheckFlag(diagnostics[0], "interva", 1, 14, 1, 21);
assertUnknownHealthcheckFlag(diagnostics[0], "interva", 1, 12, 1, 21);

// empty name
diagnostics = validate("FROM alpine\nHEALTHCHECK -- CMD ls");
assert.equal(diagnostics.length, 1);
assertUnknownHealthcheckFlag(diagnostics[0], "", 1, 12, 1, 14);

// empty value
diagnostics = validate("FROM alpine\nHEALTHCHECK --interva= CMD ls");
assert.equal(diagnostics.length, 1);
assertUnknownHealthcheckFlag(diagnostics[0], "interva", 1, 14, 1, 21);
assertUnknownHealthcheckFlag(diagnostics[0], "interva", 1, 12, 1, 21);

// no equals sign
diagnostics = validate("FROM alpine\nHEALTHCHECK --interva CMD ls");
assert.equal(diagnostics.length, 1);
assertUnknownHealthcheckFlag(diagnostics[0], "interva", 1, 14, 1, 21);
assertUnknownHealthcheckFlag(diagnostics[0], "interva", 1, 12, 1, 21);

// flags are case-sensitive
diagnostics = validate("FROM alpine\nHEALTHCHECK --INTERVAL=30s CMD ls");
assert.equal(diagnostics.length, 1);
assertUnknownHealthcheckFlag(diagnostics[0], "INTERVAL", 1, 14, 1, 22);
assertUnknownHealthcheckFlag(diagnostics[0], "INTERVAL", 1, 12, 1, 22);

diagnostics = validate("FROM alpine\nHEALTHCHECK --RETRIES=3 CMD ls");
assert.equal(diagnostics.length, 1);
assertUnknownHealthcheckFlag(diagnostics[0], "RETRIES", 1, 14, 1, 21);
assertUnknownHealthcheckFlag(diagnostics[0], "RETRIES", 1, 12, 1, 21);

diagnostics = validate("FROM alpine\nHEALTHCHECK --START-PERIOD=0s CMD ls");
assert.equal(diagnostics.length, 1);
assertUnknownHealthcheckFlag(diagnostics[0], "START-PERIOD", 1, 14, 1, 26);
assertUnknownHealthcheckFlag(diagnostics[0], "START-PERIOD", 1, 12, 1, 26);

diagnostics = validate("FROM alpine\nHEALTHCHECK --TIMEOUT=30s CMD ls");
assert.equal(diagnostics.length, 1);
assertUnknownHealthcheckFlag(diagnostics[0], "TIMEOUT", 1, 14, 1, 21);
assertUnknownHealthcheckFlag(diagnostics[0], "TIMEOUT", 1, 12, 1, 21);
});

it("flag no value", function() {
Expand Down Expand Up @@ -2505,7 +2520,7 @@ describe("Docker Validator Tests", function() {
diagnostics,
[ ValidationCode.UNKNOWN_HEALTHCHECK_FLAG, ValidationCode.UNKNOWN_TYPE ],
[ assertUnknownHealthcheckFlag, assertHealthcheckTypeUnknown ],
[ [ "intervul", 1, 14, 1, 22 ], [ "TEST", 1, 27, 1, 31 ]]);
[ [ "intervul", 1, 12, 1, 22 ], [ "TEST", 1, 27, 1, 31 ]]);
});
});

Expand All @@ -2519,7 +2534,7 @@ describe("Docker Validator Tests", function() {
diagnostics,
[ ValidationCode.UNKNOWN_HEALTHCHECK_FLAG, ValidationCode.ARGUMENT_REQUIRES_AT_LEAST_ONE ],
[ assertUnknownHealthcheckFlag, assertHEALTHCHECKRequiresAtLeastOneArgument ],
[ [ "interva", 1, 14, 1, 21 ], [ 1, 0, 1, 11 ]]);
[ [ "interva", 1, 12, 1, 21 ], [ 1, 0, 1, 11 ]]);

// empty value
diagnostics = validate("FROM alpine\nHEALTHCHECK --interva=");
Expand All @@ -2528,7 +2543,7 @@ describe("Docker Validator Tests", function() {
diagnostics,
[ ValidationCode.UNKNOWN_HEALTHCHECK_FLAG, ValidationCode.ARGUMENT_REQUIRES_AT_LEAST_ONE ],
[ assertUnknownHealthcheckFlag, assertHEALTHCHECKRequiresAtLeastOneArgument ],
[ [ "interva", 1, 14, 1, 21 ], [ 1, 0, 1, 11 ]]);
[ [ "interva", 1, 12, 1, 21 ], [ 1, 0, 1, 11 ]]);

// value specified
diagnostics = validate("FROM alpine\nHEALTHCHECK --interva=30s");
Expand All @@ -2537,7 +2552,7 @@ describe("Docker Validator Tests", function() {
diagnostics,
[ ValidationCode.UNKNOWN_HEALTHCHECK_FLAG, ValidationCode.ARGUMENT_REQUIRES_AT_LEAST_ONE ],
[ assertUnknownHealthcheckFlag, assertHEALTHCHECKRequiresAtLeastOneArgument ],
[ [ "interva", 1, 14, 1, 21 ], [ 1, 0, 1, 11 ]]);
[ [ "interva", 1, 12, 1, 21 ], [ 1, 0, 1, 11 ]]);
});
});
});
Expand Down

0 comments on commit b4634f4

Please sign in to comment.