Naming conventions - linting #16455

RaananW · 2025-04-07T18:24:16Z

If you have any questions, contact me!

Draft to give everyone time to review and present me with questions.

NOTE - lint currently fails due to javascript out of heap memory. The last few commits are just attempts to get the CI to play nicely. Once I have the solution I will clean the config file and the command running eslint

…eadability

bjsplat · 2025-04-09T15:37:25Z

Visualization tests for WebGPU

https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/refs/pull/16455/merge/testResults/webgpuplaywright/index.html

bjsplat · 2025-04-09T15:39:53Z

Reviewer - this PR has made changes to one or more package.json files.

bjsplat · 2025-04-09T15:58:48Z

Audio test results:

https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/refs/pull/16455/merge/testResults/audioV2/index.html

bjsplat · 2025-04-09T16:00:12Z

You have changed file(s) that made possible changes to the sandbox.
You can test the sandbox snapshot here:

https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/SANDBOX/refs/pull/16455/merge/

bjsplat · 2025-04-09T16:00:24Z

You have made possible changes to the playground.
You can test the snapshot here:

https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/PLAYGROUND/refs/pull/16455/merge/

The snapshot playground with the CDN snapshot (only when available):

https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/PLAYGROUND/refs/pull/16455/merge/?snapshot=refs/pull/16455/merge

Note that neither Babylon scenes nor textures are uploaded to the snapshot directory, so some playgrounds won't work correctly.

bjsplat · 2025-04-09T16:00:25Z

You have made possible changes to one or more graph tools. To test the snapshots:

https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/TOOLS/refs/pull/16455/merge/nodeEditor/
https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/TOOLS/refs/pull/16455/merge/nodeGeometryEditor/
https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/TOOLS/refs/pull/16455/merge/nodeRenderGraphEditor/
https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/TOOLS/refs/pull/16455/merge/guiEditor/

bjsplat · 2025-04-09T16:10:04Z

Visualization tests for WebGPU

https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/refs/pull/16455/merge/testResults/webgpuplaywright/index.html

bjsplat · 2025-04-09T16:11:24Z

WebGL2 visualization test reporter:

https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/refs/pull/16455/merge/testResults/webgl2playwright/index.html

docEdub

I looked over the audio engine changes and they all look good to me. Thanks!

bghgary

I reviewed some files (up to camera inputs, skipping the audio files). There are many similar issues, so I'm going to stop until we decide what to do with them.

bghgary · 2025-04-09T19:28:32Z

packages/dev/addons/src/htmlMesh/htmlMeshRenderer.ts

@@ -10,8 +10,8 @@ import type { Observer } from "core/Misc/observable";
 import { Logger } from "core/Misc/logger";
 import type { AbstractEngine } from "core/Engines";

-const _positionUpdateFailMessage = "Failed to update html mesh renderer position due to failure to get canvas rect.  HtmlMesh instances may not render correctly";
-const babylonUnitsToPixels = 100;
+const _PositionUpdateFailMessage = "Failed to update html mesh renderer position due to failure to get canvas rect.  HtmlMesh instances may not render correctly";


Does this need an underscore?

~~No, this was an old rule that was changed and will be reverted tomorrow. Same with all of those that I didn't revert~~
Sorry - no, we don't need the underscore, but those had underscore already. No underscore was added, it is just the unchanged defined naming convention. I will remove all underscores from non-exported global members to stay consistent and will remove the underscore from the rule

packages/dev/addons/src/htmlMesh/pointerEventsCaptureBehavior.ts

bghgary · 2025-04-09T19:35:19Z

packages/dev/addons/src/htmlMesh/htmlMeshRenderer.ts

@@ -297,7 +297,7 @@ export class HtmlMeshRenderer {
    }

    // prettier-ignore
-    protected _getCameraCSSMatrix(matrix: Matrix): string {
+    protected _getCameraCssMatrix(matrix: Matrix): string {


Our coding standards doc (not public) says to use all caps for initialisms. Can linting even tell the difference between initialisms and abbreviations? Maybe we need to modify the doc.

Abbreviations are an interesting topic that I will be happy to discuss. We do support abbreviations, but they need to be manually added to the lint file. Let's talk about it tomorrow.

Our coding standards doc (not public) says to use all caps for initialisms.

Not the new docs, which states:

Abbreviations [Strict] case (e.g. getMeshById, not getMeshByID)

I don't know what new docs you mean, but abbreviations are not the same as initialisms.

You are right, but that was what it meant. we can discuss this. Anyhow, abbreviations are supported (and configured). You can see it in the linting file (the first line).
StringPascalCase for example, does not support abbreviations. so It should be HtmlCssSupportId. However, with known and supported abbreviations (or initialisms :-) ) we fallback to CamelCase

packages/dev/core/src/Animations/animatable.core.ts

packages/dev/core/src/Buffers/storageBuffer.ts

packages/dev/core/src/Cameras/Inputs/arcRotateCameraGamepadInput.ts

bjsplat · 2025-04-10T16:20:54Z

Reviewer - this PR has made changes to one or more package.json files.

bjsplat · 2025-04-10T16:33:08Z

Graph tools CI has failed you can find the test results at:

https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/TOOLS/refs/pull/16455/merge/testResults/

bjsplat · 2025-04-10T16:34:45Z

You have made possible changes to the playground.
You can test the snapshot here:

https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/PLAYGROUND/refs/pull/16455/merge/

The snapshot playground with the CDN snapshot (only when available):

https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/PLAYGROUND/refs/pull/16455/merge/?snapshot=refs/pull/16455/merge

Note that neither Babylon scenes nor textures are uploaded to the snapshot directory, so some playgrounds won't work correctly.

bjsplat · 2025-04-10T16:34:46Z

Audio test results:

https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/refs/pull/16455/merge/testResults/audioV2/index.html

bjsplat · 2025-04-10T16:35:16Z

You have changed file(s) that made possible changes to the sandbox.
You can test the sandbox snapshot here:

https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/SANDBOX/refs/pull/16455/merge/

bjsplat · 2025-04-10T16:46:05Z

WebGL2 visualization test reporter:

https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/refs/pull/16455/merge/testResults/webgl2playwright/index.html

bjsplat · 2025-04-10T16:46:27Z

Visualization tests for WebGPU

https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/refs/pull/16455/merge/testResults/webgpuplaywright/index.html

bjsplat · 2025-04-10T16:48:03Z

Reviewer - this PR has made changes to one or more package.json files.

bjsplat · 2025-04-10T17:02:29Z

Audio test results:

https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/refs/pull/16455/merge/testResults/audioV2/index.html

bjsplat · 2025-04-10T17:02:39Z

You have made possible changes to the playground.
You can test the snapshot here:

https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/PLAYGROUND/refs/pull/16455/merge/

The snapshot playground with the CDN snapshot (only when available):

https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/PLAYGROUND/refs/pull/16455/merge/?snapshot=refs/pull/16455/merge

Note that neither Babylon scenes nor textures are uploaded to the snapshot directory, so some playgrounds won't work correctly.

bjsplat · 2025-04-10T17:02:42Z

You have changed file(s) that made possible changes to the sandbox.
You can test the sandbox snapshot here:

https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/SANDBOX/refs/pull/16455/merge/

bjsplat · 2025-04-10T17:03:29Z

You have made possible changes to one or more graph tools. To test the snapshots:

https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/TOOLS/refs/pull/16455/merge/nodeEditor/
https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/TOOLS/refs/pull/16455/merge/nodeGeometryEditor/
https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/TOOLS/refs/pull/16455/merge/nodeRenderGraphEditor/
https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/TOOLS/refs/pull/16455/merge/guiEditor/

bjsplat · 2025-04-10T17:13:42Z

Visualization tests for WebGPU

https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/refs/pull/16455/merge/testResults/webgpuplaywright/index.html

bjsplat · 2025-04-10T17:14:18Z

WebGL2 visualization test reporter:

https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/refs/pull/16455/merge/testResults/webgl2playwright/index.html

ryantrem · 2025-04-14T18:56:03Z

.eslintrc.js

@@ -1,3 +1,110 @@
+// want to add a new allowed abbreviation? Feer not! Just add it to the regex below.


Suggested change

// want to add a new allowed abbreviation? Feer not! Just add it to the regex below.

// want to add a new allowed abbreviation? Fear not! Just add it to the regex below.

ryantrem · 2025-04-14T19:04:00Z

packages/dev/addons/src/htmlMesh/fitStrategy.ts

@@ -20,7 +20,7 @@ const FitStrategyContain: FitStrategyType = {
        sizingElement.style.width = `${width}px`;
        sizingElement.style.height = `${height}px`;

-        const [childWidth, childHeight] = [scalingElement!.offsetWidth, scalingElement!.offsetHeight];
+        const [childWidth, childHeight] = [scalingElement.offsetWidth, scalingElement.offsetHeight];


Are unnecessary non-null assertions now flagged?

ryantrem · 2025-04-14T19:05:08Z

packages/dev/core/src/Behaviors/Meshes/pointerDragBehavior.ts

@@ -49,11 +49,13 @@ export class PointerDragBehavior implements Behavior<AbstractMesh> {
     * Get or set the currentDraggingPointerId
     * @deprecated Please use currentDraggingPointerId instead
     */
+    // eslint-disable-next-line @typescript-eslint/naming-convention


We do this for existing public APIs that don't follow the naming conventions, but for new stuff it should be enforced, is that right?

ryantrem · 2025-04-14T19:07:08Z

packages/tools/viewer/src/viewer.ts

@@ -861,7 +862,7 @@ export class Viewer implements IDisposable {
            },
            suspendRendering: () => this._suspendRendering(),
            markSceneMutated: () => this._markSceneMutated(),
-            pick: (screenX: number, screenY: number) => this._pick(screenX, screenY),
+            pick: async (screenX: number, screenY: number) => this._pick(screenX, screenY),


Why is async needed here?

ryantrem · 2025-04-14T19:08:39Z

packages/tools/viewer-configurator/src/components/babylonViewer/viewer.tsx

@@ -6,7 +6,7 @@ import { Logger } from "core/Misc/logger";
 import { ConfigureCustomViewerElement } from "viewer/viewerElement";
 import "viewer";

-interface HTML3DElementAttributes extends React.DetailedHTMLProps<React.HTMLAttributes<HTMLElement>, HTMLElement> {
+interface IHTML3DElementAttributes extends React.DetailedHTMLProps<React.HTMLAttributes<HTMLElement>, HTMLElement> {


Hmm, this gets weird where we have to decide whether it makes more sense to align with Babylon naming conventions or React naming conventions.

ryantrem · 2025-04-14T19:10:03Z

packages/tools/viewer-configurator/src/hooks/observableHooks.ts

@@ -10,6 +10,7 @@ import { useEffect, useState } from "react";
 * @param eventNames The names of the events to listen for.
 * @returns The current value of the accessor.
 */
+// eslint-disable-next-line @typescript-eslint/naming-convention
 export function useEventfulState<T>(accessor: () => T, element: HTMLElement | null | undefined, ...eventNames: string[]): T {


These functions are really only intended for internal use within the Configurator, so I would think it would be fine to update the casing.

sebavan · 2025-04-15T10:19:35Z

packages/dev/addons/src/htmlMesh/pointerEventsCaptureBehavior.ts

-    document.removeEventListener("pointermove", onPointerMove);
-    document.removeEventListener("touchstart", onPointerMove);
-    _scene = null;
+const SoStopCaptureOnEnter = () => {


Should be Do… :-)

RaananW added 30 commits February 24, 2025 17:16

update naming conventions and method names for consistency

6ab0cb5

fix: disable naming convention linting for specific interfaces and enums

5861be5

Merge remote-tracking branch 'upstream/master' into namingStandard

acb1b81

Continue with eslint naming stuff!

9089342

some more linting work linting

afebad2

refactor: replace Array constructors with array literals for better r…

e64b818

…eadability

More linting work

583deb7

Adjusting rules

d3a34e0

fix linting rule

f95fa2a

More linting

0043066

Auto-fixable

2ebfa4f

Slowly getting there...

d66c325

further adjustments

0e6eaf7

adjusting config

92b5ff7

Merge remote-tracking branch 'upstream/master' into namingStandard

6d5d016

turn off some stuff

093a8da

update package lock

e6f28cc

Linting is fun!

46fd307

linting linting linting

6643443

a rolling linting fun

2c2aea4

Yeah!

7573314

back to normal

6e83e48

some fixes

7ff47d6

some mroe fixes

0eac3be

unneeded code removal

fd6cdc1

Merge remote-tracking branch 'upstream/master' into namingStandard

17598ba

fixing some stuff

79d6dd4

Starting linting GUI

3d0d765

GUI done

c1668b3

auto-fixes dev folder

540b423

docEdub reviewed Apr 9, 2025

View reviewed changes

bghgary reviewed Apr 9, 2025

View reviewed changes

RaananW added 3 commits April 10, 2025 13:26

remove underscore from global members

b2b80a8

Variable fix (custom abbreviation in lint file)

bf25c8f

Merge remote-tracking branch 'upstream/master' into namingStandard

305a808

Allow underscore for now, needs to be discussed

82327ee

ryantrem reviewed Apr 14, 2025

View reviewed changes

sebavan reviewed Apr 15, 2025

View reviewed changes

		@@ -1,3 +1,110 @@
		// want to add a new allowed abbreviation? Feer not! Just add it to the regex below.

Naming conventions - linting #16455

Are you sure you want to change the base?

Naming conventions - linting #16455

Conversation

RaananW commented Apr 7, 2025 • edited Loading

bjsplat commented Apr 9, 2025

bjsplat commented Apr 9, 2025

bjsplat commented Apr 9, 2025

bjsplat commented Apr 9, 2025

bjsplat commented Apr 9, 2025

bjsplat commented Apr 9, 2025

bjsplat commented Apr 9, 2025

bjsplat commented Apr 9, 2025

docEdub left a comment

Choose a reason for hiding this comment

bghgary left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RaananW Apr 9, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RaananW Apr 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bjsplat commented Apr 10, 2025

bjsplat commented Apr 10, 2025

bjsplat commented Apr 10, 2025

bjsplat commented Apr 10, 2025

bjsplat commented Apr 10, 2025

bjsplat commented Apr 10, 2025

bjsplat commented Apr 10, 2025

bjsplat commented Apr 10, 2025

bjsplat commented Apr 10, 2025

bjsplat commented Apr 10, 2025

bjsplat commented Apr 10, 2025

bjsplat commented Apr 10, 2025

bjsplat commented Apr 10, 2025

bjsplat commented Apr 10, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RaananW commented Apr 7, 2025 •

edited

Loading

RaananW Apr 9, 2025 •

edited

Loading

RaananW Apr 10, 2025 •

edited

Loading