Add Multiple Y Field Selection to Plot Wizard

[julieg18]

2/4 main <- #4786 <- this <- #4797 <- #4798

Demo

Screen.Recording.2023-10-10.at.4.45.11.PM.mov

Part of #4654

[julieg18]

In plot examples, you typically use brackets for a file's multiple fields:

  - test_vs_train_loss:
      x: epoch
      y:
        training_data.csv: [test_loss, train_loss]

But our yaml library only turns arrays into - lists:

  - test_vs_train_loss:
      x: epoch
      y:
        training_data.csv: 
          - test_loss
          - train_loss

[mattseddon]

[Q] Does this format still work? I think this is fine.

[julieg18]

[Q] Does this format still work? I think this is fine.

Yes, this format still works!

[julieg18]

I considered making x and y have similar types but decided to keep them different to show that x is only one field while y has multiple. Can update the x type if preferred though :)

[mattseddon]

[Q] Are we going to allow selecting multiple x values?

[julieg18]

[Q] Are we going to allow selecting multiple x values?

Apologies, forgot to update #4654. Will do that now but in summary, quick picks do not retain the order that the user selects the items. Since we can't detect the order the user wants to add the values, it looks like were gonna just want to allow x to be a single value.

[mattseddon]

👍🏻. NW we can wait to see if someone asks for it. Did you confirm this is not possible using both the window.createQuickPick and window.showQuickPick APIs?

[julieg18]

Did you confirm this is not possible using both the window.createQuickPick and window.showQuickPick APIs?

Yes, as far as I can tell. None of the options show a way to change the order of the selected values. I also looked into some functions that get called on select but they didn't give us enough information for us to easily detect the order.

[mattseddon]

How about something like this using createQuickPick?:

  new Promise(resolve => {
    const quickPick = createQuickPick(items, selectedItems, {
      ...DEFAULT_OPTIONS,
      canSelectMany: true,
      ...options
    })

    let orderedSelection: T[] = []
    quickPick.onDidChangeSelection(selectedItems => {
      const itemValues: T[] = []
      for (const item of selectedItems) {
        if (!orderedSelection.includes(item.value)) {
          orderedSelection.push(item.value)
        }
        itemValues.push(item.value)
      }
      orderedSelection = orderedSelection.filter(item =>
        itemValues.includes(item)
      )
    })

    quickPick.onDidAccept(() => {
      resolve(orderedSelection)
      quickPick.dispose()
    })

    quickPick.onDidHide(() => {
      resolve(undefined)
      quickPick.dispose()
    })

    quickPick.show()
  })

I used quickPickLimitedValues as a base.

[julieg18]

That looks like that could work, thanks! I'll try using this type of quick pick for y fields and update x to have the same type as y so we can allow multiple x fields in a followup :)

[sroy3]

Why not test the opposite with Array.isArray(prevFileValue) instead? Array.isArray() is better for type inference.

It also feels unnecessary to create a new array twice when prevFileValue is a string. Since you are creating formattedFields here, you can just use push without worrying about mutating a variable from elsewhere.

[julieg18]

Why not test the opposite with Array.isArray(prevFileValue) instead? Array.isArray() is better for type inference.

Typescript doesn't recognize Array.isArray as a proper array check (microsoft/TypeScript#17002). For example, this causes an error:

    if (Array.isArray(formattedFields[file])) {
      formattedFields[key].push(key) // Error: Property 'push' does not exist on type 'string | string[]'.
      continue
    }

Which is why I rely on string check instead of creating some kind of extra util for array checks.

[julieg18]

It also feels unnecessary to create a new array twice when prevFileValue is a string. Since you are creating formattedFields here, you can just use push without worrying about mutating a variable from elsewhere.

Similar to the previous comment, I chose not to rely on push since according to typescript, formattedFields[key] may be a string or array so I would need to add some as statements:

    if (!formattedFields[file]) {
      formattedFields[file] = key
      continue
    }

    const prevFileValue = formattedFields[file]
    if (Array.isArray(formattedFields[file])) {
      ;(formattedFields[key] as string[]).push(key)
      continue
    }

    formattedFields[file] = [prevFileValue as string, key]

If the above is preferred as more readable though, I'm happy to update :)

[julieg18]

After looking at this with fresh eyes today, I'm going to go ahead and adjust the code to look similar to the example above. Even though it does have as statements, I think it's more readable than my current solution.

[codeclimate]

Code Climate has analyzed commit 39d255e and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 96.1% (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.