Fix/formatting by andy9a9 · Pull Request #6 · andy9a9/vscode-devicetree

andy9a9 · 2026-05-10T17:07:52Z

Fix multiple formatting issues:

do not normalize labels inside quoted strings
- labels like label = "yellow:status"; have been split

fix handling list of properties

try to do the same formatting style for separated properties

fsl,pins = <MX8MP_IOMUXC_SPDIF_RX__GPIO5_IO04	0x6>,
           <MX8MP_IOMUXC_NAND_CE0_B__GPIO3_IO01	0x6>;

fsl,pins = <MX8MP_IOMUXC_SPDIF_RX__GPIO5_IO04	0x6,
            MX8MP_IOMUXC_NAND_CE0_B__GPIO3_IO01	0x6>;

unify comment style in tests

mkessler001 · 2026-05-14T08:52:49Z

    }
 });

+/** Expand tabs to spaces for visual-column comparisons (tabSize=8). */


You mix different commenting styles.

/** Comment */

/* Comment */

/**

Comment
*/

I would decide for one and stick to it.

Also, I would not document default values like tabsize = 8 in the function comment. It just duplicates the info from the next line and it increases the effort to keep things in sync.

🤦 -> unified and removed info about size

mkessler001 · 2026-05-14T08:57:25Z

 });

+/** Expand tabs to spaces for visual-column comparisons (tabSize=8). */
+function expandTabs(s: string, tabSize = 8): string {


Use words for variables; it makes the code easier to read.

Suggested change

function expandTabs(s: string, tabSize = 8): string {

function expandTabs(line: string, tabSize = 8): string {

it's not the line, it's characters, so changed to chars

mkessler001 · 2026-05-14T09:04:35Z

+        assert.ok(!result.includes('"yellow: status"'), 'Colon in string should not have space added');
+    });
+
+    test('Should handle whitespace-separated arrays', async () => {


The name of the test is not descriptive. What does "handle" mean? I don't know what this test is doing by just reading the name.

Alternative: "Should not split whitespace-separated arrays into new lines"

mkessler001 · 2026-05-14T09:06:56Z

+        assert.ok(!result.match(/brightness-levels.*\n.*0\n.*1\n.*2/), 'Should not split digits');
+    });
+
+    test('Should handle macro function calls in arrays', async () => {


Same here. Rename the test.

mkessler001 · 2026-05-14T09:07:14Z

+        assert.ok(!result.match(/P\s+D\s+O\s+_\s+F/), 'Should not split macro name into characters');
+    });
+
+    test('Should handle long pinctrl macros', async () => {


Same here. Rename the test.

mkessler001 · 2026-05-14T12:28:47Z

+        // Block comment should be preserved
+        assert.ok(result.includes('M.2 reference clock selection'), 'Block comment should be preserved');


I would move this check before the other check so that it aligns with the name of the test (preserve block comment AND keep alignment)

moved to top of this function

mkessler001 · 2026-05-14T12:37:12Z

+            return processed.join('');
+        }).join('\n');
+
+        // Normalize node references and addresses


In the long-term, it probably makes sense to split each regex replacement into its own function. Regex expressions are hard to read/understand, and having them separated into their own functions makes them also testable.

I know, but there is lot regex-based function, so I wanted to keep them some-how grouped

mkessler001 · 2026-05-14T12:39:22Z

+        const hasCommas = this.hasSignificantCommas(trimmed);
+
+        if (!hasCommas) {
+            return trimmed.trim() ? [trimmed] : [];
+        }


Suggested change

const hasCommas = this.hasSignificantCommas(trimmed);

if (!hasCommas) {

return trimmed.trim() ? [trimmed] : [];

}

if (!this.hasSignificantCommas(trimmed)) {

return trimmed.trim() ? [trimmed] : [];

}

mkessler001 · 2026-05-14T12:41:01Z

+     * Check if a value string has significant commas (not inside parentheses or brackets)
+     * @param val The value string to check
+     * @returns True if there are commas that separate values
+     */


Explain a bit more what a significat commas is. It would make sense to give examples in the description.

description added

mkessler001 · 2026-05-14T12:47:03Z

+     * @param col Target column for alignment
+     * @returns String of tabs and/or spaces to reach the target column
+     */
+    private alignmentForColumn(col: number): string {


Use verb-based function names consistently.

normalize...

format...

calculate...

Here, use align... or use the already used format...

I would also recommend ordering the functions alphabetically.

ok, function renamed, but I wanted to keep the order based on usage not on name

Copilot

Pull request overview

This PR addresses several DTS formatter issues: preserving colons inside quoted string values (e.g. "yellow:status"), generalizing multi-line property formatting so whitespace-separated cell arrays (such as fsl,pins lists) align consistently and don't get split character-wise, distinguishing commas that act as value separators from commas nested inside (...) or <...>, and adding spaces-mode handling for values that originally contained tabs. It also unifies JSDoc-style helper comments in tests to single-line comments and adds significant test coverage for the new behavior.

Changes:

Skip label-colon normalization inside quoted strings in normalizeInput and split node-reference/address normalization into a separate step.
Add hasSignificantCommas and a whitespace-separated multi-line branch in formatMultiLineProperty, plus a new formatWhitespaceSeparatedLines helper and calculateAlignmentForColumn utility; also force spaces-mode values containing tabs to multi-line formatting.
Replace /** doc helpers with line comments in test files and add extensive new tests for strings-with-colons, brightness-levels arrays, PDO macros, and fsl,pins tab/spaces alignment.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File	Description
src/formatter.ts	Per-line label normalization that skips quoted strings; new whitespace-separated multi-line value handling with column alignment; spaces-mode fall-through when values contain tabs.
src/test/formatter.test.ts	New `expandTabs` helper, renamed existing test labels, and large new test suites covering string colons, PDO macros, iomuxc macros, and `fsl,pins` alignment in tab and spaces modes.
src/test/diagnostics.test.ts	Converts a JSDoc helper comment to a line comment (no behavior change).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

fix(formatter): do not normalize labels inside quoted strings

606fbc8

andy9a9 requested a review from mkessler001 May 10, 2026 17:08

mkessler001 reviewed May 14, 2026

View reviewed changes

andy9a9 force-pushed the fix/formatting branch from 6782329 to 91b2985 Compare May 15, 2026 13:03

fix(formatter): fix handling list of properties

5f8e8fc

andy9a9 force-pushed the fix/formatting branch from 91b2985 to 5f8e8fc Compare May 15, 2026 13:30

andy9a9 requested a review from mkessler001 May 15, 2026 13:31

andy9a9 requested a review from Copilot May 30, 2026 11:58

Copilot started reviewing on behalf of andy9a9 May 30, 2026 11:58 View session

Copilot AI reviewed May 30, 2026

View reviewed changes

	function expandTabs(s: string, tabSize = 8): string {
	function expandTabs(line: string, tabSize = 8): string {

		// Block comment should be preserved
		assert.ok(result.includes('M.2 reference clock selection'), 'Block comment should be preserved');

Conversation

andy9a9 commented May 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

andy9a9 commented May 10, 2026 •

edited

Loading