fix: recognise options that are overwritten unnecessarily by SChetwynd · Pull Request #12 · zwave-js/vscode-config-editor

SChetwynd · 2025-07-05T15:36:46Z

Changes:

Add new util function to compare JSON to AST isJSONDifferentToAST.
Use isJSONDifferentToAST function to compare changes after edit.
Add test files to build when not in production
Correct import of Mocha and Glob in test suite

Options have been overwritten and one of the properties is different, only indicates that the property is overwritten:

Options have been overwritten and all of the properties are the same, shows as an error:

(^^ I created these examples by changing the master_template, those changes are not included in the PR)

Add test files to build when not in production Correct import of Mocha and Glob in test suite

Add new util function to compare JSON to AST. Use isJSONDifferentToAST function to compare changes after edit. ✅ Closes: 7811

SChetwynd · 2025-07-05T15:42:56Z

The changes I've made to get the tests working might be out of scope for the issue which I'm trying to resolve, so I've structured my commits so that if required I can drop the changes to the test and the new tests and only include the change which resolves the issue. Or I could extract them into a new PR.

AlCalzone

Just a few nitpicks, otherwise nothing to add

Copilot

Pull Request Overview

This PR adds functionality to detect when options or properties imported via JSON AST are overridden unnecessarily, integrates the new comparison util into the diagnostics, and fixes test suite imports and build inclusion.

Introduce isJSONDifferentToAST and use it in importOverrideDiagnostics to distinguish real overrides from identical ones
Update test suite imports (Mocha, glob) and add tests for both scenarios
Modify esbuild.js to include tests in development builds

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
src/astUtils.ts	Added `isJSONDifferentToAST` helper and AST checks
src/diagnostics/importOverrideDiagnostics.ts	Use new util to flag unnecessary overrides
src/test/suite/index.ts	Switch to default imports for `glob` and `Mocha`
src/test/suite/diagnostics/importOverrideDiagnostics.test.ts	Expanded tests; fixed import paths
src/test/suite/astUtils.test.ts	Added comprehensive AST <-> JSON comparison tests
esbuild.js	Include test files in non-production builds

Comments suppressed due to low confidence (3)

src/test/suite/astUtils.test.ts:120

[nitpick] This test description duplicates an earlier one but tests the opposite case (JSON longer than AST). Please update it to accurately reflect the scenario.

		test("returns true if ast array is longer than json array", () => {

esbuild.js:20

[nitpick] Bundling test files into the extension build can bloat output. Consider using a separate build step or directory for tests.

	entryPoints: ["./src/extension.ts", ...testFiles],

src/test/suite/index.ts:1

[nitpick] Switching to default imports requires esModuleInterop (or allowSyntheticDefaultImports) in tsconfig. Please verify your compiler settings to avoid runtime import errors.

import glob from "glob";

Copilot · 2025-07-08T14:04:47Z

+				.map((n) => {
+					return [
+						getPropertyNameFromNode(n),
+						n.parent.valueNode,


Using n.parent.valueNode returns the AST node rather than the JSON value. Consider using getPropertyValueFromNode(n) to pass the actual JSON value into the diagnostics.

Suggested change

n.parent.valueNode,

getPropertyValueFromNode(n),

Hmm I missed this change. The change itself is fine, so ignore Copilot's review here. Given that the array now contains the AST node and not its value, the variable in line 82 should be renamed to valueNode.

I'm wondering if the array could be reduced to just the property name , and the ASTNode.

Line 94 could be changed to: value: getPropertyValueFromNode(propNode),.

And line 85 to if (!isJSONDifferentToAST(originalValue, propNode.parent.valueNode))

Hmm that wouldn't change the other type issue with the value possibly being undefined. Let's just stick to renaming then and making sure we pass the actual value to the diagnostic and not the AST node (see other comment).

I've changed the variable name as suggested

Copilot · 2025-07-08T14:04:47Z

-			const isUnchanged = value === originalValue;

-			if (isUnchanged) {
+			if (value && !isJSONDifferentToAST(originalValue, value)) {


[nitpick] The value check is always truthy for an ASTNode. You can remove the redundant value && guard to simplify the condition.

Suggested change

if (value && !isJSONDifferentToAST(originalValue, value)) {

if (!isJSONDifferentToAST(originalValue, value)) {

This nitpick seems correct, please change.

Typescript complains when I make this change, value is set to n.parent.valueNode, and n.parent is typed as an ASTNode which doesn't always have a valueNode even if logically it must have one.

I could change the isJSONDifferentToAST to accept undefined as the second param, which would remove the need to check if the value is defined here.

Ok, let's keep this one as is then.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

AlCalzone

@SChetwynd Sorry, there are a few more things I missed. I already committed the spelling fixes, so make sure to pull first.

The comments are referring to the Copilot review comments above, in case the context is not clear.

AlCalzone · 2025-07-08T14:10:39Z

+				.map((n) => {
+					return [
+						getPropertyNameFromNode(n),
+						n.parent.valueNode,


Hmm I missed this change. The change itself is fine, so ignore Copilot's review here. Given that the array now contains the AST node and not its value, the variable in line 82 should be renamed to valueNode.

AlCalzone · 2025-07-08T14:11:24Z

-			const isUnchanged = value === originalValue;

-			if (isUnchanged) {
+			if (value && !isJSONDifferentToAST(originalValue, value)) {


This nitpick seems correct, please change.

change return value when import is overwritten, but not unnecessarily

AlCalzone

Thanks a lot!

SChetwynd added 3 commits July 4, 2025 21:15

test: 💍 fix test runner

60113e5

Add test files to build when not in production Correct import of Mocha and Glob in test suite

fix: 🐛 recognises options which have been overwritten

07eaec1

Add new util function to compare JSON to AST. Use isJSONDifferentToAST function to compare changes after edit. ✅ Closes: 7811

test: 💍 new function and importOverrideDiagnostics

b35f22c

SChetwynd mentioned this pull request Jul 5, 2025

VSCode extension: Recognize options that are overwritten unnecessarily zwave-js/zwave-js#7811

Closed

AlCalzone requested changes Jul 7, 2025

View reviewed changes

Comment thread esbuild.js

Comment thread src/astUtils.ts Outdated

Comment thread src/astUtils.ts

Comment thread src/astUtils.ts

refactor: 💡 remove unnecessary variable, add comment

9a46d4e

AlCalzone changed the title ~~fix: Recognise options that are overwritten unnecessarily (#7811)~~ fix: recognise options that are overwritten unnecessarily Jul 8, 2025

AlCalzone requested a review from Copilot July 8, 2025 14:02

Copilot AI reviewed Jul 8, 2025

View reviewed changes

fix typos

12d18ad

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

AlCalzone requested changes Jul 8, 2025

View reviewed changes

refactor: 💡 rename variable, change return value

3fa5c22

change return value when import is overwritten, but not unnecessarily

AlCalzone approved these changes Jul 9, 2025

View reviewed changes

AlCalzone merged commit 4f4f8d0 into zwave-js:main Jul 9, 2025
2 checks passed

	if (value && !isJSONDifferentToAST(originalValue, value)) {
	if (!isJSONDifferentToAST(originalValue, value)) {

Uh oh!

Conversation

SChetwynd commented Jul 5, 2025 • edited by AlCalzone Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes:

Uh oh!

SChetwynd commented Jul 5, 2025

Uh oh!

AlCalzone left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI Jul 8, 2025

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 Jul 8, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SChetwynd Jul 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlCalzone left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Uh oh!

AlCalzone left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

SChetwynd commented Jul 5, 2025 •

edited by AlCalzone

Loading

SChetwynd Jul 8, 2025 •

edited

Loading

AlCalzone left a comment •

edited

Loading