Fix ESLint warnings (1) #1060
Fix ESLint warnings (1) #1060
Conversation
|
Wow, wonderful and great initiate, we like ! :) |
|
💯 |
|
By the way still working on the e2e test, which apparently run fine here on fresh develop branches, ... scratch scratch |
| @@ -7,7 +7,7 @@ import Form from '@rjsf/core'; | |||
| import { | |||
| showGphlWorkflowParametersDialog, | |||
| gphlWorkflowSubmitParameters, | |||
| } from '../actions/gphl_workflow'; | |||
| } from '../actions/gphlWorkflow'; | |||
There was a problem hiding this comment.
No ESLint warning here, it's just for consistency.
| @@ -1,3 +1,4 @@ | |||
| /* eslint-disable sonarjs/no-duplicate-string */ | |||
There was a problem hiding this comment.
sonarjs/no-duplicate-string: I'm disabling the rule in a couple of files — the rule can be a bit noisy but helps spot duplicated identifiers and such, so I prefer to not disable it globally
| @@ -141,7 +142,7 @@ export const INITIAL_STATE = { | |||
| energyScanElements: [], | |||
| }; | |||
|
|
|||
| export default (state = INITIAL_STATE, action) => { | |||
| function beamlineReducer(state = INITIAL_STATE, action = {}) { | |||
There was a problem hiding this comment.
- import/no-anonymous-default-export: everything exported should be named
- default-param-last: since
statehas a default value,actionmust also have a default value.
| @@ -1,5 +1,4 @@ | |||
| /* eslint-disable import/no-anonymous-default-export */ | |||
| const initialState = { | |||
| const INITIAL_STATE = { | |||
There was a problem hiding this comment.
Constant case by convention
| }; | ||
| } | ||
| case 'ADD_SAMPLES_TO_QUEUE': { | ||
| const sampleIDList = action.samplesData.map((s) => s.sampleID); | ||
| return { ...state, queue: state.queue.concat(sampleIDList) }; | ||
| return { ...state, queue: [...state.queue, ...sampleIDList] }; |
There was a problem hiding this comment.
prefer-spread: foo.concat(bar) => [...foo, ...bar] – this is just a stylistic rule, useful to ensure consistency in the codebase
| @@ -26,7 +26,7 @@ export default (state = initialState, action) => { | |||
| const newNodes = []; | |||
|
|
|||
| action.sampleOrder.forEach((sampleID) => { | |||
| if (action.sampleList.hasOwnProperty(sampleID)) { | |||
| if (sampleID in action.sampleList) { | |||
There was a problem hiding this comment.
no-prototype-builtins reports that foo.hasOwnProperty("bar") should be replaced with Object.prototype.hasOwnProperty.call(foo, "bar") to account for potential edge cases.
However, in the cases reported, hasOwnProperty can simply be replaced with the in operator, since we're only dealing with plain objects.
| sampleList[sampleID].tasks.concat(action.samplesData[i].tasks); | ||
| } else { | ||
| sampleList[sampleID].tasks = action.samplesData[i].tasks; | ||
| } |
There was a problem hiding this comment.
no-unused-expressions — ternaries are expressions that resolve to a value, so using a ternary just to trigger side effects is counter-intuitive. if/else blocks are better suited.
This change actually reveals a more serious issue, which is that the array returned by concat is not used (concat does not mutate the arrays) — so the expression in the if block has no effect at all. Not sure what the correct behaviour should be. ESLint warns about it, so I decided to leave it as is for now.
| selectedShapes: [], | ||
| }; | ||
| } | ||
| case 'CLEAR_ALL': |
There was a problem hiding this comment.
no-duplicated-branches — duplicated case blocks
| if ( | ||
| (record.state === 1 && !taskCollapsed) || | ||
| (record.state >= 2 && taskCollapsed) | ||
| ) { |
There was a problem hiding this comment.
marcus-oscarsson
force-pushed
the
develop
branch
from
a436cc3
to
4652da2
Compare
|
👍 Thanks alot Axel ! |
No better time than the present! I'll split things up a bit so it's easier to digest. Starting count is 365 warnings.
In this PR, I'm knocking down 36 warnings in the reducers, bringing the count down to 329.
I'll add comments to point to the relevant rules.