Skip to content

Commit 5d973ad

Browse files
authored
feat(es-lint-rule) eslint no-unused-param rule more strict (#3499)
* Modifying the eslint no-unused-param rule to be a bit more strict while still allowing unused parameters for handlers. This allows us to catch a bit more cases when the parameter truly isn't used and shouldn't be part of the function signature. * Standardized error handling between querying via WMS or via WFS and improved the error handling in general as well for the WMS layer type in particular
1 parent 28df86f commit 5d973ad

48 files changed

Lines changed: 447 additions & 300 deletions

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

.github/copilot-instructions.md

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -325,6 +325,24 @@ See [time-dimension.md](../docs/programming/time-dimension.md) for the full arch
325325

326326
GeoView uses a lightweight typed delegate event system (see [event-helper.md](../docs/programming/event-helper.md)). Classes own private handler arrays (`#onXxxHandlers`) and expose `onXxx()`/`offXxx()` subscribe/unsubscribe methods. Events are emitted via `EventHelper.emitEvent()`. Controllers subscribe in `onHook()` and unsubscribe in `onUnhook()`.
327327

328+
**Handler parameter naming convention (required):** For EventHelper delegate handlers, always name parameters `sender` and `event`.
329+
330+
- Use `(sender, event)` even if one parameter is not used in the body.
331+
- `sender` and `event` are exception names in the no-unused-parameter ESLint rule.
332+
- Apply this convention in controllers, domains, APIs, and any callback wired to `onXxx()` EventHelper delegates.
333+
334+
```typescript
335+
// ✅ Good
336+
mapViewer.onMapMoveEnd((sender, event) => {
337+
logger.logDebug(event.lonlat);
338+
});
339+
340+
// ❌ Avoid
341+
mapViewer.onMapMoveEnd((map, e) => {
342+
logger.logDebug(e.lonlat);
343+
});
344+
```
345+
328346
## TypeScript Conventions
329347

330348
### Type Safety (Strict Enforcement)
@@ -352,6 +370,25 @@ const layers: string[] = [];
352370
useState<TypeBasemapProps[]>([]);
353371
```
354372

373+
- **Prefer optional property syntax for optional attributes/properties**: For class attributes and type/interface properties that may be absent, prefer `prop?: Type` over `prop?: Type | undefined`.
374+
375+
```typescript
376+
// ✅ Good: concise optional property
377+
interface LayerConfig {
378+
layerName?: string;
379+
}
380+
381+
// ❌ Avoid in most cases: redundant undefined union on optional property
382+
interface LayerConfig {
383+
layerName?: string | undefined;
384+
}
385+
386+
// ✅ Exception: use this only when presence-vs-absence must be distinguished
387+
interface LayerState {
388+
layerName: string | undefined;
389+
}
390+
```
391+
355392
- **Avoid name collisions**: Use `GVLayer` not `Layer` when OpenLayers has a `Layer` class
356393

357394
### String Concatenation

docs/programming/best-practices.md

Lines changed: 40 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,27 @@ async fetchMetadata(id: string): Promise<void> {
3838
const handleClick = useCallback((): void => { ... }, []);
3939
```
4040
41+
Prefer optional property syntax (`?:`) for class attributes and type/interface properties that may be absent. In most cases, this is clearer than using an explicit `| undefined` union.
42+
43+
```ts
44+
// Preferred in most cases
45+
class LayerInfo {
46+
layerName?: string;
47+
}
48+
49+
type TypeLayerConfig = {
50+
sourceUrl?: string;
51+
};
52+
53+
// Use only when presence-vs-absence must be distinguished
54+
class LayerState {
55+
// Property is always present, but value can be undefined
56+
sourceUrl: string | undefined;
57+
}
58+
```
59+
60+
Use `property: Type | undefined` only when that distinction is intentional and required by behavior (for example: serialization differences, merge semantics, or APIs that depend on checking whether a key exists).
61+
4162
## 2- Avoid using variable names that are too short.
4263
4364
It is difficult to know what a variable with the name `e` refers to. Is it an `element`, an `event` or anything else whose name starts
@@ -249,7 +270,25 @@ In classes, functions should be ordered in the following way:
249270
- static private
250271
- event types
251272
252-
## 12- React Performance Patterns
273+
## 12- EventHelper handler parameter naming
274+
275+
When subscribing to events emitted through our `EventHelper` delegates, handler methods should use the parameter names `sender` and `event`.
276+
277+
This naming is required for consistency and readability across the codebase. These names are also treated as exceptions in our no-unused-parameter ESLint rule, so they should be used even when one of the arguments is not consumed in the implementation.
278+
279+
```ts
280+
// ✅ Good: EventHelper delegate naming convention
281+
this.getMapViewer().onMapMoveEnd((sender, event): void => {
282+
logger.logDebug('Map moved', event.lonlat);
283+
});
284+
285+
// ✅ Also good when one parameter is intentionally unused
286+
this.getMapViewer().onMapInit((sender, event): void => {
287+
initializeSomething();
288+
});
289+
```
290+
291+
## 13- React Performance Patterns
253292
254293
### useMemo Naming Convention
255294

packages/eslint.config.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -107,7 +107,7 @@ export default [
107107
'no-useless-constructor': 'off',
108108
'@typescript-eslint/no-useless-constructor': 'error',
109109
'no-unused-vars': 'off',
110-
'@typescript-eslint/no-unused-vars': 'warn',
110+
'@typescript-eslint/no-unused-vars': ["warn", { "args": "all", "argsIgnorePattern": "^_|^event$|^sender$" }],
111111
'@typescript-eslint/no-unnecessary-type-assertion': 'warn',
112112
'@typescript-eslint/no-inferrable-types': 'warn',
113113
'no-use-before-define': 'off',

packages/geoview-core/src/api/types/layer-schema-types.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -468,7 +468,7 @@ export type RCSLayerConfig = {
468468
/**
469469
* The display name of the layer (English/French). This overrides the default name coming from the GeoCore API.
470470
*/
471-
geoviewLayerName?: string | undefined;
471+
geoviewLayerName?: string;
472472

473473
/** Initial settings to apply to the GeoCore layer at creation time. */
474474
initialSettings?: TypeLayerInitialSettings;
@@ -491,7 +491,7 @@ export type GeoPackageLayerConfig = {
491491
metadataAccessPath: string;
492492

493493
/** The display name of the layer. This overrides the default name coming from the GeoCore API. */
494-
geoviewLayerName?: string | undefined;
494+
geoviewLayerName?: string;
495495

496496
/** Initial settings to apply to the layer at creation time. */
497497
initialSettings?: TypeLayerInitialSettings;
@@ -514,7 +514,7 @@ export type ShapefileLayerConfig = {
514514
metadataAccessPath: string;
515515

516516
/** The display name of the layer. This overrides the default name coming from the GeoCore API. */
517-
geoviewLayerName?: string | undefined;
517+
geoviewLayerName?: string;
518518

519519
/** Initial settings to apply to the layer at creation time. */
520520
initialSettings?: TypeLayerInitialSettings;

packages/geoview-core/src/api/types/map-schema-types.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1049,7 +1049,7 @@ export type TypeFeatureInfoEntry = {
10491049
uid?: string;
10501050
feature?: Feature<Geometry>;
10511051
geometry?: Geometry;
1052-
extent: Extent | undefined;
1052+
extent?: Extent;
10531053
featureIcon?: string;
10541054
fieldInfo: Partial<Record<string, TypeFieldEntry>>;
10551055
nameField?: string;

packages/geoview-core/src/core/components/common/layer-list.tsx

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ export interface LayerListEntry {
3535
/** Number of features in the layer. */
3636
numOffeatures?: number;
3737
/** Array of feature info entries. */
38-
features?: TypeFeatureInfoEntry[] | undefined;
38+
features?: TypeFeatureInfoEntry[];
3939
/** Unique DOM id for the layer list item. */
4040
layerUniqueId?: string;
4141
/** Whether the layer item is disabled. */

packages/geoview-core/src/core/components/data-table/export-button.tsx

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ interface ExportButtonProps {
1717
layerPath: string;
1818
rows: DataTableRow[];
1919
columns: MRTColumnDef<DataTableRow>[];
20-
children?: ReactElement | undefined;
20+
children?: ReactElement;
2121
}
2222

2323
/** The columns to remove from the data when exporting */

packages/geoview-core/src/core/components/guide/guide.tsx

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,7 @@ export const Guide = memo(function GuidePanel({ containerType }: GuideType): JSX
6868
* Handles search state changes from GuideSearch component.
6969
*/
7070
const handleSearchStateChange = useCallback(
71-
(newSearchTerm: string, newHighlightFunction: (content: string, sectionIndex: number) => string): void => {
71+
(_newSearchTerm: string, newHighlightFunction: (content: string, sectionIndex: number) => string): void => {
7272
setHighlightFunction(() => newHighlightFunction);
7373
},
7474
[]

packages/geoview-core/src/core/components/layers/delete-undo-button.tsx

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -128,12 +128,10 @@ export function DeleteUndoButton(props: DeleteUndoButtonProps): JSX.Element {
128128
}
129129
};
130130

131-
// eslint-disable-next-line @typescript-eslint/no-unused-vars
132131
const handleDeleteClick = (event: MouseEvent): void => {
133132
performDelete(false);
134133
};
135134

136-
// eslint-disable-next-line @typescript-eslint/no-unused-vars
137135
const handleUndoClick = (event: MouseEvent): void => {
138136
performUndo(false);
139137
};

packages/geoview-core/src/core/components/layers/right-panel/layer-opacity-control/layer-opacity-control.tsx

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,7 @@ export function LayerOpacityControl({ layerPath }: LayerOpacityControlProps): JS
7474
* @param updateStore - Should the store be updated.
7575
*/
7676
const handleSliderChange = useCallback(
77+
// eslint-disable-next-line @typescript-eslint/no-unused-vars
7778
(value: number | number[], activeThumb: number, updateStore = false): void => {
7879
const val = (Array.isArray(value) ? value[0] : value) / 100;
7980
const newValue = Math.min(val, layerParentOpacity);

0 commit comments

Comments
 (0)