HTML parser cleanup, comment attachment fixes, and buildCodeFrame improvements
RENAME
packages/@romefrontend/cli-diagnostics/buildMessageCodeFrame.ts
packages/@romefrontend/cli-diagnostics/buildCodeFrame.ts
@@ -85,16 +85,7 @@ function createPointer(
}
}
-export default function buildMessageCodeFrame(
- {sourceText, lines: allLines, start, end, markerMessage = ""}: {
- sourceText: string;
- lines: ToLines;
- start?: Position;
- end?: Position;
- markerMessage?: string;
- },
-): string {
- if (allLines.length === 0 || start === undefined || end === undefined) {
+function formatMarker(markerMessage: string): string {
if (markerMessage === "") {
return "";
} else {
@@ -102,14 +93,67 @@ export default function buildMessageCodeFrame(
}
}
- const startLineIndex = ob1Coerce1To0(start.line);
- let endLineIndex = ob1Coerce1To0(end.line);
+export default function buildCodeFrame(
+ {
+ sourceText,
+ lines: allLines,
+ truncateLines,
+ start,
+ end,
+ type,
+ markerMessage = "",
+ }: {
+ sourceText: string;
+ lines: ToLines;
+ type: "pointer" | "all";
+ truncateLines?: number;
+ start?: Position;
+ end?: Position;
+ markerMessage?: string;
+ },
+): {
+ frame: string;
+ truncated: boolean;
+} {
+ // Bail if we have negative line references, we have no lines, or we expected positions and don't have one
+ let shouldBail = allLines.length === 0;
+ if (type === "pointer" && (start === undefined || end === undefined)) {
+ shouldBail = true;
+ }
+ if (start !== undefined && start.line === ob1Number1Neg1) {
+ shouldBail = true;
+ }
+ if (end !== undefined && end.line === ob1Number1Neg1) {
+ shouldBail = true;
+ }
+ if (shouldBail) {
+ return {
+ frame: formatMarker(markerMessage),
+ truncated: false,
+ };
+ }
+
+ // Whether we truncated lines
+ let truncated = false;
+
+ const startLineIndex =
+ start === undefined ? ob1Number0 : ob1Coerce1To0(start.line);
+ let endLineIndex =
+ end === undefined
+ ? ob1Coerce0(allLines.length - 1)
+ : ob1Coerce1To0(end.line);
// Increase the amount of lines we should show for "context"
- let contextStartIndex = ob1Coerce0(
+ let contextStartIndex =
+ start === undefined
+ ? startLineIndex
+ : ob1Coerce0(
Math.max(0, ob1Get0(startLineIndex) - CODE_FRAME_CONTEXT_LINES),
);
- let contextEndIndex = ob1Coerce0(
+ let contextEndIndex =
+ end === undefined
+ ? endLineIndex
+ : ob1Coerce0(
Math.min(
allLines.length - 1,
ob1Get0(endLineIndex) + CODE_FRAME_CONTEXT_LINES,
@@ -145,9 +189,10 @@ export default function buildMessageCodeFrame(
let pointer: undefined | string;
// If this is within the highlighted line range
- const shouldHighlight: boolean = i >= startLineIndex && i <= endLineIndex;
+ const shouldHighlight: boolean =
+ type === "pointer" && i >= startLineIndex && i <= endLineIndex;
- if (shouldHighlight) {
+ if (shouldHighlight && start !== undefined && end !== undefined) {
if (i === startLineIndex && i === endLineIndex) {
// Only line in the selection
pointer = createPointer(
@@ -199,10 +244,16 @@ export default function buildMessageCodeFrame(
});
maxVisibleLineNo = ob1Get0(i) + 1;
+
+ if (truncateLines !== undefined && maxVisibleLineNo >= truncateLines) {
+ truncated = true;
+ break;
+ }
}
// If we have too many lines in our selection, then collapse them to an ellipsis
- const pruned = formattedLines.length > MAX_CODE_FRAME_LINES + 2;
+ const pruned =
+ type === "pointer" && formattedLines.length > MAX_CODE_FRAME_LINES + 2;
if (pruned) {
const start = formattedLines.slice(0, HALF_MAX_CODE_FRAME_LINES);
const end = formattedLines.slice(-HALF_MAX_CODE_FRAME_LINES);
@@ -220,16 +271,11 @@ export default function buildMessageCodeFrame(
}
// If there's no lines to target then return the normal marker
- if (
- formattedLines.length === 0 ||
- end.line === ob1Number1Neg1 ||
- start.line === ob1Number1Neg1
- ) {
- if (markerMessage === "") {
- return "";
- } else {
- return `<nobr>${markerMessage}</nobr>`;
- }
+ if (formattedLines.length === 0) {
+ return {
+ frame: formatMarker(markerMessage),
+ truncated: false,
+ };
}
// Calculate max size of gutter, this is the maximum visible line plus the futter length plus the frame indent
@@ -242,7 +288,7 @@ export default function buildMessageCodeFrame(
String(maxVisibleLineNo).length + GUTTER.length + CODE_FRAME_INDENT.length;
// If what the marker is highlighting equals the marker message then it's redundant so don't show the message
- if (markerMessage !== "") {
+ if (markerMessage !== "" && start !== undefined && end !== undefined) {
const text = sourceText.slice(ob1Get0(start.index), ob1Get0(end.index));
if (
cleanEquivalentString(text) ===
@@ -266,7 +312,10 @@ export default function buildMessageCodeFrame(
result.push(`${CODE_FRAME_INDENT}${selection.pointer}`);
}
- return joinNoBreak(result);
+ return {
+ frame: joinNoBreak(result),
+ truncated,
+ };
}
// Build up the line we display when source lines are omitted
@@ -293,5 +342,14 @@ export default function buildMessageCodeFrame(
}
}
- return joinNoBreak(result);
+ if (truncated) {
+ result.push(
+ `${omittedLine} <dim><number>${maxVisibleLineNo - truncateLines!}</number> more lines truncated</dim>`,
+ );
+ }
+
+ return {
+ truncated,
+ frame: joinNoBreak(result),
+ };
}

MODIFY
packages/@romefrontend/cli-diagnostics/constants.ts
packages/@romefrontend/cli-diagnostics/constants.ts
@@ -14,6 +14,7 @@ export const HALF_MAX_CODE_FRAME_LINES = MAX_CODE_FRAME_LINES / 2;
export const CODE_FRAME_CONTEXT_LINES = 2;
// Constants that influence truncation
-export const MAX_CODE_LENGTH = 500;
-export const MAX_PATCH_LINES = 50;
+export const MAX_CODE_LENGTH = 5_000;
+export const MAX_CODE_LINES = 150;
+export const MAX_PATCH_LINES = 150;
export const MAX_LOG_LENGTH = 5_000;

MODIFY
packages/@romefrontend/cli-diagnostics/highlightCode.ts
packages/@romefrontend/cli-diagnostics/highlightCode.ts
@@ -220,7 +220,7 @@ function highlightHTML({input, path}: AnsiHighlightOptions): string {
case "Identifier":
return {
- type: prev !== undefined && prev.type === "Less"
+ type: prev !== undefined && prev.type === "TagStartOpen"
? "tag"
: "attr-name",
};
@@ -228,9 +228,10 @@ function highlightHTML({input, path}: AnsiHighlightOptions): string {
case "String":
return {type: "attr-value"};
- case "Greater":
- case "Less":
- case "Slash":
+ case "TagEndOpen":
+ case "TagEnd":
+ case "TagSelfClosing":
+ case "TagStartOpen":
return {type: "punctuation"};
default:

MODIFY
packages/@romefrontend/cli-diagnostics/printAdvice.ts
packages/@romefrontend/cli-diagnostics/printAdvice.ts
@@ -25,7 +25,7 @@ import {
import {Position} from "@romefrontend/parser-core";
import {ToLines, toLines} from "./utils";
import buildPatchCodeFrame from "./buildPatchCodeFrame";
-import buildMessageCodeFrame from "./buildMessageCodeFrame";
+import buildCodeFrame from "./buildCodeFrame";
import {
escapeMarkup,
markupTag,
@@ -35,7 +35,7 @@ import {DiagnosticsPrinterFlags} from "./types";
import {ob1Number0Neg1} from "@romefrontend/ob1";
import DiagnosticsPrinter, {DiagnosticsPrinterFileSources} from "./DiagnosticsPrinter";
import {AbsoluteFilePathSet, createUnknownFilePath} from "@romefrontend/path";
-import {MAX_CODE_LENGTH, MAX_LOG_LENGTH} from "./constants";
+import {MAX_CODE_LENGTH, MAX_CODE_LINES, MAX_LOG_LENGTH} from "./constants";
import {Diffs, diffConstants} from "@romefrontend/string-diff";
import {removeCarriageReturn} from "@romefrontend/string-utils";
import {serializeCLIFlags} from "@romefrontend/cli-flags";
@@ -302,12 +302,14 @@ function printCode(
): PrintAdviceResult {
const {reporter} = opts;
- const truncated =
+ const truncatedLength =
!opts.flags.verboseDiagnostics && item.code.length > MAX_CODE_LENGTH;
- let code = truncated ? item.code.slice(0, MAX_CODE_LENGTH) : item.code;
+ let code = truncatedLength ? item.code.slice(0, MAX_CODE_LENGTH) : item.code;
- const frame = buildMessageCodeFrame({
+ const {frame, truncated: truncatedLines} = buildCodeFrame({
+ type: "all",
sourceText: code,
+ truncateLines: MAX_CODE_LINES,
lines: toLines({
input: code,
path: createUnknownFilePath("inline"),
@@ -321,13 +323,13 @@ function printCode(
reporter.logAll(frame);
- if (truncated) {
+ if (truncatedLength) {
printTruncated(reporter, item.code.length - MAX_CODE_LENGTH);
}
return {
printed: true,
- truncated,
+ truncated: truncatedLines || truncatedLength,
};
}
@@ -378,7 +380,8 @@ function printFrame(
sourceText = "";
}
- const frame = buildMessageCodeFrame({
+ const {frame, truncated} = buildCodeFrame({
+ type: "pointer",
sourceText,
lines,
start,
@@ -390,7 +393,10 @@ function printFrame(
}
reporter.logAll(frame);
- return DID_PRINT;
+ return {
+ printed: true,
+ truncated,
+ };
}
function printStacktrace(

MODIFY
packages/@romefrontend/compiler/transforms/defaultHooks/index.ts
packages/@romefrontend/compiler/transforms/defaultHooks/index.ts
@@ -124,7 +124,7 @@ export const commentInjector = createHook<
const {id} = comment;
if (id === undefined) {
- commentWithId = path.context.comments.addComment(comment);
+ commentWithId = path.context.comments.createComment(comment);
} else {
// This comment already has an id so update it
commentWithId = {

MODIFY
packages/@romefrontend/css-parser/parse.ts
packages/@romefrontend/css-parser/parse.ts
@@ -415,7 +415,7 @@ export const createCSSParser = createParser((ParserCore) =>
index = ob1Add(index, 1);
}
this.registerComment(
- this.comments.addComment({
+ this.comments.createComment({
type: "CommentBlock",
value,
}),

MODIFY
packages/@romefrontend/diagnostics/descriptions/htmlParser.ts
packages/@romefrontend/diagnostics/descriptions/htmlParser.ts
@@ -11,6 +11,7 @@ export const htmlParser = createDiagnosticsCategory({
INCORRECT_CLOSING_TAG_NAME: (expected: string, got: string) => ({
message: markup`Expected to close ${expected} but found ${got}`,
}),
+ UNOPENED_TAG: "Ending tag with no opening tag",
UNCLOSED_TAG: (tagName: string, openLocation: DiagnosticLocation) => ({
message: markup`Unclosed ${tagName} tag`,
advice: [

MODIFY
packages/@romefrontend/html-parser/index.test.ts
packages/@romefrontend/html-parser/index.test.ts
@@ -15,6 +15,7 @@ const promise = createFixtureTests(async (fixture, t) => {
const inputContent = removeCarriageReturn(inputFile.content.toString());
const ast = parseHTML({
+ inlineDiagnosticsSource: true,
input: inputContent,
path: filename,
});

MODIFY
packages/@romefrontend/html-parser/index.ts
packages/@romefrontend/html-parser/index.ts
@@ -22,14 +22,18 @@ import {Number0, ob1Add, ob1Get0, ob1Inc} from "@romefrontend/ob1";
import {isEscaped} from "@romefrontend/string-utils";
import {isSelfClosingTagName} from "./tags";
import {descriptions} from "@romefrontend/diagnostics";
-import {consumeComment} from "@romefrontend/html-parser/utils.ts";
type Tokens = BaseTokens & {
Text: ValueToken<"Text", string>;
- Slash: SimpleToken<"Slash">;
- Less: SimpleToken<"Less">;
+ // <
+ TagStartOpen: SimpleToken<"TagStartOpen">;
+ // />
+ TagSelfClosing: SimpleToken<"TagSelfClosing">;
+ // >
+ TagEnd: SimpleToken<"TagEnd">;
+ // </
+ TagEndOpen: SimpleToken<"TagEndOpen">;
Equals: SimpleToken<"Equals">;
- Greater: SimpleToken<"Greater">;
Identifier: ValueToken<"Identifier", string>;
String: ValueToken<"String", string>;
Comment: ValueToken<"Comment", string>;
@@ -60,12 +64,13 @@ function isTextChar(char: string, index: Number0, input: string): boolean {
return !isTagStartChar(index, input);
}
-function isTagComment(index: Number0, input: string): boolean {
- const first = input[ob1Get0(index)];
- const second = input[ob1Get0(index) + 1];
- const third = input[ob1Get0(index) + 2];
- const fourth = input[ob1Get0(index) + 3];
- return first === "<" && second === "!" && third === "-" && fourth === "-";
+function isntCommentEnd(char: string, index: Number0, input: string): boolean {
+ const isCommentEnd =
+ char === "-" &&
+ !isEscaped(index, input) &&
+ input[ob1Get0(index) + 1] === "-" &&
+ input[ob1Get0(index) + 2] === ">";
+ return !isCommentEnd;
}
const createHTMLParser = createParser((ParserCore, ParserWithRequiredPath) =>
@@ -94,10 +99,6 @@ const createHTMLParser = createParser((ParserCore, ParserWithRequiredPath) =>
const char = this.getInputCharOnly(index);
if (!escaped && state.inTagHead) {
- if (char === " ") {
- return this.lookahead(ob1Inc(index));
- }
-
if (char === "=") {
return {
state,
@@ -105,10 +106,10 @@ const createHTMLParser = createParser((ParserCore, ParserWithRequiredPath) =>
};
}
- if (char === "/") {
+ if (char === "/" && this.getInputCharOnly(index, 1)) {
return {
state,
- token: this.finishToken("Slash"),
+ token: this.finishToken("TagSelfClosing", ob1Add(index, 2)),
};
}
@@ -143,30 +144,56 @@ const createHTMLParser = createParser((ParserCore, ParserWithRequiredPath) =>
...state,
inTagHead: false,
},
- token: this.finishToken("Greater"),
+ token: this.finishToken("TagEnd"),
};
}
}
- if (isTagComment(index, this.input)) {
- const startingIndex = ob1Add(index, 2);
- const [endIndex, value] = consumeComment(startingIndex, this.input);
+ if (
+ this.getInputCharOnly(index) === "<" &&
+ this.getInputCharOnly(index, 1) === "!" &&
+ this.getInputCharOnly(index, 2) === "-" &&
+ this.getInputCharOnly(index, 3) === "-"
+ ) {
+ // Skip <!--
+ const start = ob1Add(index, 4);
+ const [value, valueEnd, overflow] = this.readInputFrom(
+ start,
+ isntCommentEnd,
+ );
+
+ // Check for unclosed comment
+ if (overflow) {
+ // TODO
+ }
+
+ // Skip -->
+ const end = ob1Add(valueEnd, 3);
+
return {
state: {
...state,
inTagHead: false,
},
- token: this.finishValueToken("Comment", value, endIndex),
+ token: this.finishValueToken("Comment", value, end),
};
}
if (isTagStartChar(index, this.input)) {
+ let token;
+
+ if (this.getInputCharOnly(index, 1) === "/") {
+ token = this.finishToken("TagEndOpen", ob1Add(index, 2));
+ } else {
+ token = this.finishToken("TagStartOpen");
+ }
+
return {
state: {
...state,
inTagHead: true,
},
- token: this.finishToken("Less"),
+ token,
};
}
@@ -222,20 +249,9 @@ const createHTMLParser = createParser((ParserCore, ParserWithRequiredPath) =>
);
}
- atTagEnd(): boolean {
- return this.matchToken("Less") && this.lookahead().token.type === "Slash";
- }
-
- atComment(): boolean {
- return this.lookahead().token.type === "Comment";
- }
-
parseTag(): HTMLElement {
const headStart = this.getPosition();
- if (this.atComment()) {
- this.parseComment();
- }
- this.expectToken("Less");
+ this.expectToken("TagStartOpen");
const attributes: HTMLElement["attributes"] = [];
const children: HTMLElement["children"] = [];
@@ -245,14 +261,15 @@ const createHTMLParser = createParser((ParserCore, ParserWithRequiredPath) =>
let selfClosing = isSelfClosingTagName(tagName);
// Parse attributes
- while (!this.matchToken("EOF") && !this.matchToken("Greater")) {
+ while (
+ !this.matchToken("EOF") &&
+ !this.matchToken("TagSelfClosing") &&
+ !this.matchToken("TagEnd")
+ ) {
const keyToken = this.getToken();
if (keyToken.type === "Identifier") {
attributes.push(this.parseAttribute());
- } else if (keyToken.type === "Slash") {
- this.nextToken();
- selfClosing = true;
} else {
throw this.unexpected({
description: descriptions.HTML_PARSER.EXPECTED_ATTRIBUTE_NAME,
@@ -260,7 +277,12 @@ const createHTMLParser = createParser((ParserCore, ParserWithRequiredPath) =>
}
}
- this.expectToken("Greater");
+ if (this.eatToken("TagSelfClosing")) {
+ selfClosing = true;
+ } else {
+ this.expectToken("TagEnd");
+ }
+
const headEnd = this.getPosition();
// Verify closing tag
@@ -268,7 +290,7 @@ const createHTMLParser = createParser((ParserCore, ParserWithRequiredPath) =>
while (
// Build children
!this.matchToken("EOF") &&
- !this.atTagEnd()
+ !this.matchToken("TagEndOpen")
) {
const child = this.parseChild();
if (child !== undefined) {
@@ -284,8 +306,7 @@ const createHTMLParser = createParser((ParserCore, ParserWithRequiredPath) =>
),
});
} else {
- this.expectToken("Less");
- this.expectToken("Slash");
+ this.expectToken("TagEndOpen");
const name = this.getToken();
if (name.type === "Identifier") {
@@ -305,7 +326,7 @@ const createHTMLParser = createParser((ParserCore, ParserWithRequiredPath) =>
});
}
- this.expectToken("Greater");
+ this.expectToken("TagEnd");
}
}
@@ -325,11 +346,13 @@ const createHTMLParser = createParser((ParserCore, ParserWithRequiredPath) =>
const start = this.getPosition();
const token = this.expectToken("Comment");
- this.comments.addComment({
+ this.registerComment(
+ this.comments.createComment({
value: token.value,
type: "CommentBlock",
loc: this.finishLoc(start),
- });
+ }),
+ );
return undefined;
}
@@ -391,7 +414,7 @@ const createHTMLParser = createParser((ParserCore, ParserWithRequiredPath) =>
const token = this.getToken();
switch (token.type) {
- case "Less":
+ case "TagStartOpen":
return this.parseTag();
case "Text":
@@ -400,8 +423,19 @@ const createHTMLParser = createParser((ParserCore, ParserWithRequiredPath) =>
case "Comment":
return this.parseComment();
- default:
- throw this.unexpected();
+ case "TagEndOpen": {
+ this.unexpectedDiagnostic({
+ description: descriptions.HTML_PARSER.UNOPENED_TAG,
+ });
+ this.nextToken();
+ return undefined;
+ }
+
+ default: {
+ this.unexpectedDiagnostic();
+ this.nextToken();
+ return undefined;
+ }
}
}

MODIFY
packages/@romefrontend/html-parser/test-fixtures/smoke/input.test.md
packages/@romefrontend/html-parser/test-fixtures/smoke/input.test.md
```javascript
HTMLRoot {
- comments: Array []
corrupt: false
diagnostics: Array []
filename: "input.html"
@@ -24,10 +23,47 @@ HTMLRoot {
line: 1
}
}
+ comments: Array [
+ CommentBlock {
+ id: "0"
+ value: " I am a comment "
+ loc: Object {
+ filename: "input.html"
+ end: Object {
+ column: 23
+ index: 23
+ line: 1
+ }
+ start: Object {
+ column: 0
+ index: 0
+ line: 1
+ }
+ }
+ }
+ CommentBlock {
+ id: "1"
+ value: " I am another comment "
+ loc: Object {
+ filename: "input.html"
+ end: Object {
+ column: 30
+ index: 210
+ line: 17
+ }
+ start: Object {
+ column: 1
+ index: 181
+ line: 17
+ }
+ }
+ }
+ ]
body: Array [
HTMLElement {
name: HTMLIdentifier {
name: "foo"
+ leadingComments: undefined
loc: Object {
filename: "input.html"
end: Object {
@@ -44,6 +80,7 @@ HTMLRoot {
}
attributes: Array []
selfClosing: false
+ leadingComments: Array ["0"]
loc: Object {
filename: "input.html"
end: Object {

DELETE
packages/@romefrontend/html-parser/utils.ts
-import {Number0, ob1Add, ob1Get} from "@romefrontend/ob1";
-
-export function getChar(index: Number0, input: string, offset = 0): string {
- const targetIndex = ob1Get(index) + offset;
- return input[targetIndex];
-}
-
-export function consumeComment(index: Number0, input: string): [Number0, string] {
- let value = "";
- while (getChar(index, input) !== ">") {
- const char = getChar(index, input);
- if (char !== "-") {
- value += getChar(index, input);
- }
- index = ob1Add(index, 1);
- }
-
- return [ob1Add(index, 1), value.trim()];
-}

MODIFY
packages/@romefrontend/js-parser/CommentsConsumer.ts
packages/@romefrontend/js-parser/CommentsConsumer.ts
@@ -74,7 +74,7 @@ export default class CommentsConsumer {
this.idToComment.delete(id);
}
- addComment(withoutId: AnyCommentOptionalId): AnyComment {
+ createComment(withoutId: AnyCommentOptionalId): AnyComment {
const withId: AnyComment = {
...withoutId,
id: this.getNextId(),

MODIFY
packages/@romefrontend/js-parser/tokenizer/index.ts
packages/@romefrontend/js-parser/tokenizer/index.ts
@@ -308,13 +308,13 @@ function pushComment(
const loc = parser.finishLocAt(opts.startPos, opts.endPos);
let comment: AnyComment;
if (opts.block) {
- comment = parser.comments.addComment({
+ comment = parser.comments.createComment({
type: "CommentBlock",
value: removeCarriageReturn(opts.text),
loc,
});
} else {
- comment = parser.comments.addComment({
+ comment = parser.comments.createComment({
type: "CommentLine",
value: opts.text,
loc,

MODIFY
packages/@romefrontend/parser-core/index.ts
packages/@romefrontend/parser-core/index.ts
@@ -52,6 +52,7 @@ import CommentsConsumer from "@romefrontend/js-parser/CommentsConsumer";
export * from "./types";
export type ParserOptions = {
+ inlineDiagnosticsSource?: boolean;
retainCarriageReturn?: boolean;
path?: string | UnknownFilePath;
mtime?: number;
@@ -150,11 +151,12 @@ export class ParserCore<
diagnosticCategory: DiagnosticCategory,
initialState: Omit<State, keyof ParserCoreState>,
) {
- const {path, mtime, offsetPosition} = opts;
+ const {path, mtime, offsetPosition, inlineDiagnosticsSource = false} = opts;
// Input information
this.path = path === undefined ? undefined : createUnknownFilePath(path);
this.filename = this.path === undefined ? undefined : this.path.join();
+ this.shouldInlineDiagnosticsSource = inlineDiagnosticsSource;
this.mtime = mtime;
this.input = normalizeInput(opts);
this.length = ob1Coerce0(this.input.length);
@@ -206,7 +208,7 @@ export class ParserCore<
path: undefined | UnknownFilePath;
filename: undefined | string;
-
+ shouldInlineDiagnosticsSource: boolean;
mtime: undefined | number;
input: string;
length: Number0;
@@ -519,7 +521,10 @@ export class ParserCore<
return {
description: metadataWithCategory,
location: {
- sourceText: this.path === undefined ? this.input : undefined,
+ sourceText: this.path === undefined ||
+ this.shouldInlineDiagnosticsSource
+ ? this.input
+ : undefined,
mtime: this.mtime,
start,
end,

HTML parser cleanup, comment attachment fixes, and buildCodeFrame improvements (#774)