Skip to content

Conversation

@precondition
Copy link
Contributor

@precondition precondition commented Nov 3, 2025

Overview

This pull request fixes a critical bug in convert_keymap_extras_header.js that caused it to fail on all inputs. It restores the full destructure bindings and adds // eslint-disable-next-line no-unused-vars directives to preserve positional correctness while avoiding linting errors.

Description

Commit 0828211 introduced a regression in the convert_keymap_extras_header.js script used for i18n keymap extras.

Several destructuring assignments (let [a, b, c] = function_call();) had commented-out elements (e.g. /*fullMatch, */) to avoid no-unused-vars warnings.

/home/username/Coding/qmk/configurator/src/i18n/keymap_extras/convert_keymap_extras_header.js
  126:12  error  'fullMatch' is assigned a value but never used  no-unused-vars
  241:10  error  'fullMatch' is assigned a value but never used  no-unused-vars
  256:21  error  'intlAlias' is assigned a value but never used  no-unused-vars
  256:48  error  'comment' is assigned a value but never used    no-unused-vars
  336:8   error  'fullMatch' is assigned a value but never used  no-unused-vars

It appears that these unused variables (which are indeed unused because only a subset of the destructured outputs of a function are used) were quickly commented out to get rid of the warning. Unfortunately, this broke the code because the variables were no longer assigned to the correct output of the function within the destructuring assignments (if you turn let [a, b, c] = function_call(); into let [a, /*b,*/ c] = function_call();, variable c will point to what was intended to be b).

There are multiple ways to deal with this problem:

Option 1: eslint disable comment

// eslint-disable-next-line no-unused-vars
const [fullMatch, intlAlias, macroExpansion] = kcAliasDefRegExp.exec(line);

(does not make it clear which variable exactly is unused)

Option 2: Ignore variables with an underscore prefix

Add a rule in eslint.config.js to ignore the no-unused-vars warning for variables that start with an underscore:

"rules": {
  "no-unused-vars": ["error", { "argsIgnorePattern": "^_", "varsIgnorePattern": "^_" }]
}

and then do this in the code:

const [_fullMatch, intlAlias, macroExpansion] = kcAliasDefRegExp.exec(line);

(makes it clear which variable exactly is unused)

Option 3: name all unused variables as _

Unfortunately, this option will not work. Consider the case of multiple ignored/unused variables in the destructured assignment of line 256 in function convertLine:

256    let [fullMatch, _, macroExpansion, _] = kcAliasDefRegExp.exec(line);

/home/username/Coding/qmk/configurator/src/i18n/keymap_extras/convert_keymap_extras_header.js
  256:40  error  Parsing error: Identifier '_' has already been declared

For now, the PR adopts option 1 but this can be discussed.

Affected functions

  • computeIntl2US
  • convertLine
  • generateSpaceCadetKcInfo

Relevant discussion thread on Discord: https://discord.com/channels/440868230475677696/867530744116543508/1434968023113928848

…warnings

Previously, several destructuring assignments had commented-out elements
(e.g. /*fullMatch, */) to avoid `no-unused-vars` warnings. This broke
the code because the variables were no longer assigned to the correct
output of the function within the destructure bindings.

This commit restores the full destructure bindings (e.g. `fullMatch`, `intlAlias`,
`comment`) and adds `// eslint-disable-next-line no-unused-vars` pragmas to
preserve positional correctness without triggering lint errors.

Affected functions:
- computeIntl2US
- convertLine
- generateSpaceCadetKcInfo
@yanfali
Copy link
Collaborator

yanfali commented Nov 4, 2025

ah ok. I see the issue you're running into. You're de-structuring into an array. since you really don't care about those have you tried this?

const [first, , third] = array;  // Skip second element
const [, , third] = array;       // Skip first two

but keep the commas; very important for proper order of positional
arguments
@precondition
Copy link
Contributor Author

ah ok. I see the issue you're running into. You're de-structuring into an array. since you really don't care about those have you tried this?

const [first, , third] = array;  // Skip second element
const [, , third] = array;       // Skip first two

I did not know about this feature of the JavaScript syntax. Implemented in commit e4efd78.

@yanfali yanfali self-assigned this Nov 7, 2025
@yanfali yanfali added the bug label Nov 7, 2025
Copy link
Collaborator

@yanfali yanfali left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm

@yanfali
Copy link
Collaborator

yanfali commented Nov 15, 2025

oh sorry. I forgot you can't merge.

@yanfali yanfali merged commit b0c7a95 into qmk:master Nov 15, 2025
2 checks passed
@precondition precondition deleted the i18n-restore-destructure-vars branch November 17, 2025 18:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants