Improve Custom Plot Creation

[julieg18]

• hide "Add Plots" button if there are no more plots to create
• hide pick custom plot type options if there are no types available

Demo

Screen.Recording.2023-03-24.at.4.41.23.PM.mov

Followup to #3526

[codeclimate]

Function checkForMetricVsParamPlotOptions has a Cognitive Complexity of 6 (exceeds 5 allowed). Consider refactoring.

[julieg18]

I tried a couple things but, as far as I can tell, as long as there as an if statement inside these two for loops, I get the complexity warning. Only solutions that come to mind that don't require the if statement require going through the entire loop which I was trying to avoid 🤔

I ended up adding a cognitive-complexity comment for now but is there a better way to do this?

[mattseddon]

In order to pass the check you need to remove the inner loop. Something like

const hasCombination = (
  plotIds: Set<string>,
  metric: string,
  params: string[]
): boolean => params.some(param => !plotIds.has(getCustomPlotId(metric, param)))

export const checkForMetricVsParamPlotOptions = (
  columns: Column[],
  customPlotOrder: CustomPlotsOrderValue[]
): boolean => {
  const { metrics, params } = getCustomPlotPathsFromColumns(columns)
  const plotIds = getCustomPlotIds(
    customPlotOrder,
    CustomPlotType.METRIC_VS_PARAM
  )

  for (const metric of metrics) {
    if (hasCombination(plotIds, metric, params)) {
      return true
    }
  }

  return false
}

However, whether or not this is more readable is debatable.

Maybe we should move complexity to 6 in both sonarjs and code climate.

WDYT?

cc @sroy3

[mattseddon]

Raised #3556 to discuss.

[shcheklein]

One option could be is to create a function that is an iterator and is producing one by one a Cartesian product of metrics and params. It will be used in two places instead of const { metrics, params } = getCustomPlotPathsFromColumns(columns) and two for loops:

for plotId in allMetricVsParamsPlotIds():
   if ...

[julieg18]

This function and the one above are the same as before, just moved from quickPick to custom.

[julieg18]

Switched the function out for an if statement so I could use FILE_SEPARATOR in custom.ts, reducing extra arguments being passed around.

[mattseddon]

In order to pass the check you need to remove the inner loop. Something like

const hasCombination = (
  plotIds: Set<string>,
  metric: string,
  params: string[]
): boolean => params.some(param => !plotIds.has(getCustomPlotId(metric, param)))

export const checkForMetricVsParamPlotOptions = (
  columns: Column[],
  customPlotOrder: CustomPlotsOrderValue[]
): boolean => {
  const { metrics, params } = getCustomPlotPathsFromColumns(columns)
  const plotIds = getCustomPlotIds(
    customPlotOrder,
    CustomPlotType.METRIC_VS_PARAM
  )

  for (const metric of metrics) {
    if (hasCombination(plotIds, metric, params)) {
      return true
    }
  }

  return false
}

However, whether or not this is more readable is debatable.

Maybe we should move complexity to 6 in both sonarjs and code climate.

WDYT?

cc @sroy3

[mattseddon]

[Q] Do we need to return early here if there are no items? Even if the path currently cannot be hit it would be as a safeguard.

[mattseddon]

[Q] Are add/remove custom plot available from the command palette? Should they be? Will they be?

[mattseddon]

Same [Q] for experiments table.

[julieg18]

[Q] Are add/remove custom plot available from the command palette? Should they be? Will they be?
Same [Q] for experiments table.

Currently, these commands are not used in the command palette or the table and we have no plans for it that I know of. As for if they should be... Adding them to the command palette could be useful but I know we want to avoid adding every possible command in the palette 🤔

[mattseddon]

but I know we want to avoid adding every possible command in the palette

I don't agree with this. I think we should add as much useful functionality as possible there. Look at what these (very popular) Git extensions do:

Screen.Recording.2023-03-28.at.9.09.28.am.mov

[julieg18]

I don't agree with this. I think we should add as much useful functionality as possible there. Look at what these (very popular) Git extensions do:

Sounds good to me! Will update #3373.

[codeclimate]

Code Climate has analyzed commit 6c49a07 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (85% is the threshold).

This pull request will bring the total coverage in the repository to 95.1% (0.0% change).

View more on Code Climate.