Skip to content

[js] fix wanrings and errors found by code quality checkers#22695

Merged
linev merged 4 commits into
root-project:masterfrom
linev:fix_js
Jun 25, 2026
Merged

[js] fix wanrings and errors found by code quality checkers#22695
linev merged 4 commits into
root-project:masterfrom
linev:fix_js

Conversation

@linev

@linev linev commented Jun 25, 2026

Copy link
Copy Markdown
Member

Changes are in JSROOT and UI5 components

Most about useless if conditions or unused functions arguments
But also real bugs where instead b variable d was used or
wrong check of function name for function type.

Fixes split by components

linev added 4 commits June 25, 2026 07:54
Several useless break and if statements
In the elements loop - when element removed one need to decrease index to continue with next element
Duplicated 'tooltip' property
Useless if statement
Wrong check for function type
Useless if statement
Duplicated 'tooltip' property
Several fixes found by static checker
@linev linev requested review from bellenot and dpiparo June 25, 2026 06:10
@linev linev self-assigned this Jun 25, 2026
Comment thread js/build/jsroot.js
const b = d.get('batch');
if (b !== undefined) {
setBatchMode(d !== 'off');
setBatchMode(b !== 'off');

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

A good way to avoid such bugs is having better names for variables...consider renaming them

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

There are too many of such places. Start changing them - one can introduce more errors

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I didn't propose to change all places at the same time, just the ones we discover over time such as this. I see no chance of introducing more errors if it's just a local variable renaming.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

"Resolving" an ongoing conversation and merging regardless is not a great practice...

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

JavaScript code changed not here - but in JSROOT repository.
Your are welcome to provide PR there.

But I see no big advantage to change variable names in existing code.

@bellenot bellenot left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@github-actions

Copy link
Copy Markdown

Test Results

    22 files      22 suites   3d 15h 0m 55s ⏱️
 3 874 tests  3 872 ✅   0 💤 2 ❌
76 494 runs  76 384 ✅ 108 💤 2 ❌

For more details on these failures, see this check.

Results for commit 742f6da.

@linev linev merged commit b55eeb1 into root-project:master Jun 25, 2026
32 of 37 checks passed
@linev linev deleted the fix_js branch June 25, 2026 10:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants