Fix counting of current line-length (#121)

* Fix counting of current line-length So it works correctly with the various settings of attributeSeparator * Move separator logic to shared function * Add comments about printWidth: 80 * Apply suggestions from code review * Make code that counts current line length clearer Add comments explaining the handling of literal attributes and normal ones * Remove note about pugPrintWidth being inaccurate
prettier · Oct 4, 2020 · 1be2bdd · 1be2bdd
1 parent 7cdeafc
commit 1be2bdd
Show file tree

Hide file tree

Showing 11 changed files with 68 additions and 30 deletions.
diff --git a/README.md b/README.md
@@ -86,7 +86,6 @@ By using versions of these standard options prefixed with `pug`, you can overrid
   Print spaces between brackets in object literals.
 - `pugPrintWidth`  
   Specify the line length that the printer will wrap on.  
-  _Currently this is not very accurate, but works._
 - `pugSemi`  
   Print semicolons at the ends of code statements.
 - `pugSingleQuote`  

diff --git a/src/printer.ts b/src/printer.ts
@@ -100,15 +100,11 @@ export class PugPrinter {
 	private result: string = '';
 
 	private currentIndex: number = 0;
+	private currentLineLength: number = 0;
 
 	private readonly indentString: string;
 	private indentLevel: number = 0;
 
-	/**
-	 * The line length starts by 1, it's not an zero based index
-	 */
-	private currentLineLength: number = 1;
-
 	private readonly quotes: "'" | '"';
 	private readonly otherQuotes: "'" | '"';
 
@@ -234,6 +230,12 @@ export class PugPrinter {
 		return !!token && possibilities.includes(token.type) !== invert;
 	}
 
+	private tokenNeedsSeparator(token: AttributeToken): boolean {
+		return this.neverUseAttributeSeparator
+			? false
+			: this.alwaysUseAttributeSeparator || /^(\(|\[|:).*/.test(token.name);
+	}
+
 	private formatDelegatePrettier(
 		val: string,
 		parser: '__vue_expression' | '__ng_binding' | '__ng_action' | '__ng_directive'
@@ -414,21 +416,24 @@ export class PugPrinter {
 			this.possibleClassPosition = this.result.length;
 			result = '(';
 			logger.debug(this.currentLineLength);
-			this.currentLineLength += 1;
 			let tempToken: AttributeToken | EndAttributesToken = this.nextToken;
 			let tempIndex: number = this.currentIndex + 1;
-			let nonPrefixAttributes: number = 0;
-			let hasPrefixAttribute: boolean = false;
-			let numAttributes: number = 0;
+			// In pug, tags can have two kind of attributes: normal attributes that appear between parantheses,
+			// and literals for ids and classes, prefixing the paranthesis, e.g.: `#id.class(attribute="value")`
+			// https://pugjs.org/language/attributes.html#class-literal
+			// https://pugjs.org/language/attributes.html#id-literal
+			// In the stream of attribute tokens, distinguish those that can be converted to literals,
+			// and count those that cannot (normal attributes) to determine the resulting line length correctly.
+			let hasLiteralAttributes: boolean = false;
+			let numNormalAttributes: number = 0;
 			while (tempToken.type === 'attribute') {
-				numAttributes++;
 				if (!this.wrapAttributes && this.wrapAttributesPattern?.test(tempToken.name)) {
 					this.wrapAttributes = true;
 				}
 				switch (tempToken.name) {
 					case 'class':
 					case 'id': {
-						hasPrefixAttribute = true;
+						hasLiteralAttributes = true;
 						const val: string = tempToken.val.toString();
 						if (isQuoted(val)) {
 							this.currentLineLength -= 2;
@@ -441,8 +446,15 @@ export class PugPrinter {
 						break;
 					}
 					default: {
-						nonPrefixAttributes += 1;
 						this.currentLineLength += tempToken.name.length;
+						if (numNormalAttributes > 0) {
+							// This isn't the first normal attribute that will appear between parantheses,
+							// add space and separator
+							this.currentLineLength += 1;
+							if (this.tokenNeedsSeparator(tempToken)) {
+								this.currentLineLength += 1;
+							}
+						}
 						logger.debug(
 							{ tokenName: tempToken.name, length: tempToken.name.length },
 							this.currentLineLength
@@ -452,35 +464,29 @@ export class PugPrinter {
 							this.currentLineLength += 1 + val.length;
 							logger.debug({ tokenVal: val, length: val.length }, this.currentLineLength);
 						}
+						numNormalAttributes++;
 						break;
 					}
 				}
 				tempToken = this.tokens[++tempIndex] as AttributeToken | EndAttributesToken;
 			}
 			logger.debug('after token', this.currentLineLength);
-			if (hasPrefixAttribute) {
-				// Remove div
+			if (hasLiteralAttributes) {
+				// Remove div as it will be replaced with the literal for id and/or class
 				if (this.previousToken?.type === 'tag' && this.previousToken.val === 'div') {
 					this.currentLineLength -= 3;
 				}
 			}
-			const hasPrefixAttributes: boolean = nonPrefixAttributes > 0;
-			if (!hasPrefixAttributes) {
-				// Remove leading brace
-				this.currentLineLength -= 1;
-			} else {
-				// Attributes are separated by commas: ', '.length === 2
-				this.currentLineLength += 2 * (nonPrefixAttributes - 1);
-
-				// Add trailing brace
-				this.currentLineLength += 1;
+			if (numNormalAttributes > 0) {
+				// Add leading and trailing parantheses
+				this.currentLineLength += 2;
 			}
 			logger.debug(this.currentLineLength);
 			if (
 				!this.wrapAttributes &&
 				(this.currentLineLength > this.options.pugPrintWidth ||
 					(this.options.pugWrapAttributesThreshold >= 0 &&
-						numAttributes > this.options.pugWrapAttributesThreshold))
+						numNormalAttributes > this.options.pugWrapAttributesThreshold))
 			) {
 				this.wrapAttributes = true;
 			}
@@ -584,10 +590,7 @@ export class PugPrinter {
 			this.currentIndex
 		);
 		if (this.previousToken?.type === 'attribute' && (!this.previousAttributeRemapped || hasNormalPreviousToken)) {
-			if (
-				!this.neverUseAttributeSeparator &&
-				(this.alwaysUseAttributeSeparator || /^(\(|\[|:).*/.test(token.name))
-			) {
+			if (this.tokenNeedsSeparator(token)) {
 				this.result += ',';
 			}
 			if (!this.wrapAttributes) {

diff --git a/tests/options/attributeSeparator/always/always.test.ts b/tests/options/attributeSeparator/always/always.test.ts
@@ -11,6 +11,8 @@ describe('Options', () => {
 			const actual: string = format(code, {
 				parser: 'pug' as any,
 				plugins: [plugin],
+				// The `.length-test` elements are tested against a `printWidth` of 80 (currently also the default):
+				printWidth: 80,
 				// @ts-expect-error
 				attributeSeparator: 'always'
 			});

diff --git a/tests/options/attributeSeparator/always/formatted.pug b/tests/options/attributeSeparator/always/formatted.pug
@@ -1,3 +1,10 @@
+.length-test(attributes1="foo", :attribute2="bar", attribute3="filler.........")
+.length-test(
+  attributes1="foo",
+  :attribute2="bar",
+  attribute3="filler.........."
+)
+
 button(type="submit", (click)="play()", disabled)
 hello-framework(
   [name]="name",

diff --git a/tests/options/attributeSeparator/always/unformatted.pug b/tests/options/attributeSeparator/always/unformatted.pug
@@ -1,3 +1,6 @@
+.length-test(attributes1="foo", :attribute2="bar", attribute3="filler.........")
+.length-test(attributes1="foo", :attribute2="bar", attribute3="filler..........")
+
 button(type="submit", (click)="play()" disabled)
 hello-framework([name]="name", [version]="version", (release)="onVersionRelease()")
 

diff --git a/tests/options/attributeSeparator/as-needed/as-needed.test.ts b/tests/options/attributeSeparator/as-needed/as-needed.test.ts
@@ -11,6 +11,8 @@ describe('Options', () => {
 			const actual: string = format(code, {
 				parser: 'pug' as any,
 				plugins: [plugin],
+				// The `.length-test` elements are tested against a `printWidth` of 80 (currently also the default):
+				printWidth: 80,
 				// @ts-expect-error
 				attributeSeparator: 'as-needed'
 			});

diff --git a/tests/options/attributeSeparator/as-needed/formatted.pug b/tests/options/attributeSeparator/as-needed/formatted.pug
@@ -1,3 +1,10 @@
+.length-test(attributes1="foo", :attribute2="bar" attribute3="filler..........")
+.length-test(
+  attributes1="foo",
+  :attribute2="bar"
+  attribute3="filler..........."
+)
+
 button(type="submit", (click)="play()" disabled)
 hello-framework(
   [name]="name",

diff --git a/tests/options/attributeSeparator/as-needed/unformatted.pug b/tests/options/attributeSeparator/as-needed/unformatted.pug
@@ -1,3 +1,6 @@
+.length-test(attributes1="foo", :attribute2="bar" attribute3="filler..........")
+.length-test(attributes1="foo", :attribute2="bar" attribute3="filler...........")
+
 button(type="submit", (click)="play()", disabled)
 hello-framework([name]="name", [version]="version", (release)="onVersionRelease()")
 

diff --git a/tests/options/attributeSeparator/none/formatted.pug b/tests/options/attributeSeparator/none/formatted.pug
@@ -1,3 +1,10 @@
+.length-test(attributes1="foo" :attribute2="bar" attribute3="filler...........")
+.length-test(
+  attributes1="foo"
+  :attribute2="bar"
+  attribute3="filler............"
+)
+
 button(type="submit" :style="style" @click="play()" disabled)
 
 nav-component(locale-relative-redirect="true" highlight="home" pin="false")

diff --git a/tests/options/attributeSeparator/none/none.test.ts b/tests/options/attributeSeparator/none/none.test.ts
@@ -11,6 +11,8 @@ describe('Options', () => {
 			const actual: string = format(code, {
 				parser: 'pug' as any,
 				plugins: [plugin],
+				// The `.length-test` elements are tested against a `printWidth` of 80 (currently also the default):
+				printWidth: 80,
 				// @ts-expect-error
 				attributeSeparator: 'none'
 			});

diff --git a/tests/options/attributeSeparator/none/unformatted.pug b/tests/options/attributeSeparator/none/unformatted.pug
@@ -1,3 +1,6 @@
+.length-test(attributes1="foo" :attribute2="bar" attribute3="filler...........")
+.length-test(attributes1="foo" :attribute2="bar" attribute3="filler............")
+
 button(type="submit", :style="style", @click="play()", disabled)
 
 nav-component(locale-relative-redirect="true", highlight="home", pin="false")