Skip to content

SysID: fix ESLint problems - #358

Open
j-w9 wants to merge 4 commits into
ArduPilot:mainfrom
j-w9:lint-SysID
Open

j-w9 wants to merge 4 commits into
ArduPilot:mainfrom
j-w9:lint-SysID

Conversation

@j-w9

@j-w9 j-w9 commented Oct 11, 2026

Copy link
Copy Markdown
Contributor

Fixes ESLint problems in SysID and removes the fixed entries from eslint-suppressions.json. None of the changes alter behaviour.

  • Declare variables that were implicitly global. Each was assigned without let/var, creating a global, but is only used within the function (or loop) that assigns it, and is not used by any other script on the page, so let does not change behaviour.
  • Declare the matrix-filling variables in each function. rows, cols, cellSelector and inputElement were implicit globals shared by setMatrixA, setMatrixB, setH0 and setH1. Each function assigns them before reading them and none calls another, so declaring them locally does not change behaviour.
  • Remove variables that are assigned but never read. temp_data in the compensation loops was an implicit global that nothing read; declaring it would only make it an unused local, so it is removed.

One commit per kind of fix; npm run lint passes.

j-w9 added 2 commits October 13, 2026 22:30
Each was assigned without let/var, creating a global, but is only used
within the function (or loop) that assigns it, and is not used by any
other script on the page, so let does not change behaviour.
rows, cols, cellSelector and inputElement were implicit globals shared
by setMatrixA, setMatrixB, setH0 and setH1. Each function assigns them
before reading them and none calls another, so declaring them locally
does not change behaviour.
@AP-Review

AP-Review commented Nov 9, 2026 •

Copy link
Copy Markdown

Deprecated — see below for the updated review.

Previous review (2026-11-09)

Automated review note — AI-generated (Claude+Codex), validated against the live diff. Please sanity-check before acting.
Verdict: COMMENT

Reviewed at head 0708bb1d90.
Full report: https://firmware.ardupilot.org/Tools/APReview/DevCallReviews/PRReviews/ardupilot/webtools/358/1.html#prWebTools-358

This PR, which declares SysID's implicit globals as locals and prunes the matching ESLint suppressions, looks correct. Base and head behave the same in browser harnesses, and the suppression counts match a fresh ESLint run.

Minor

  • Commit 4e56ce5 message: the body is copied from the FilterTool commit and mentions dbl/triple and pruning suppressions, but this commit only removes the two temp_data assignments. Reword it to cover just those.
  • eslint-suppressions.json:126: the 3 remaining SysID.js no-undef hits are pre-existing Python-style keyword arguments at SysID/SysID.js:17 and :20 (deps=false, keep_going=true). In JS these create globals and pass positional booleans. A possible follow-up is an options object, as line 26 already uses. Not introduced by this PR.

j-w9 added 2 commits November 8, 2026 22:48
temp_data in the two compensation loops was an implicit global that
nothing read; declaring it only made it an unused local. Drop it and
prune its suppressions.
@AP-Review

Copy link
Copy Markdown

Automated review note — AI-generated (Claude+Codex), validated against the live diff. Please sanity-check before acting.
Verdict: ACCEPT

Reviewed at head daa5c55f7f.
Full report: https://firmware.ardupilot.org/Tools/APReview/DevCallReviews/PRReviews/ardupilot/webtools/358/2.html#prWebTools-358

This PR, which turns SysID's implicit globals into locals and prunes the matching ESLint suppressions, looks good to merge. Base and head behave the same in harness tests, and the suppression counts match a fresh ESLint run.

Since the last review the only change is the reworded body of commit 8d22e3a, which no longer has the copied dbl/triple text. The code is identical.

Minor (2)
  • Commit 8d22e3a message: the body still ends "Drop it and prune its suppressions", but the pruning is in daa5c55. Optionally end it at "Drop it."
  • eslint-suppressions.json:126: keyword-style micropip args (still open). This PR does not introduce them. With Pyodide 0.26.1's micropip 0.6.0, SysID/SysID.js:17 deps=false actually becomes keep_going=false and deps stays true. The options-object form used at line 26 would fix it in a follow-up.

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.

3 participants