Add error handling for Plot Wizard plots with x fields #4798
Conversation
|
Code Climate has analyzed commit 135582a and detected 0 issues on this pull request. The test coverage on the diff in this pull request is 63.3% (85% is the threshold). This pull request will bring the total coverage in the repository to 94.9%. View more on Code Climate. |
julieg18
changed the title
x dict
julieg18
changed the base branch from
main
to
allow-plot-wizard-multi-x-selection
…dling-for-plot-wizard-field-dict
…dling-for-plot-wizard-field-dict
| @@ -588,17 +617,109 @@ describe('pickPlotConfiguration', () => { | |||
| mockedQuickPickOne.mockResolvedValue('simple') | |||
| mockedGetInput.mockResolvedValue('simple_plot') | |||
| mockedQuickPickUserOrderedValues | |||
| .mockResolvedValueOnce([ | |||
There was a problem hiding this comment.
This test was actually incorrectly written. See https://github.com/iterative/vscode-dvc/pull/4797/files#r1355869332
| @@ -580,6 +580,35 @@ describe('pickPlotConfiguration', () => { | |||
| expect(mockedQuickPickUserOrderedValues).toHaveBeenCalledTimes(2) | |||
| expect(noFieldsResult).toStrictEqual(undefined) | |||
| }) | |||
|
|
|||
| it('should show a toast if user selects more than one key in a file for an x field', async () => { | |||
There was a problem hiding this comment.
At this point, we've got around 20 different tests in this describe block. I'd like to break group these tests to make things more readable but I'll do it in a separate pr since that would involve touching the whole file. Updated #4654.
There was a problem hiding this comment.
[Q] What is the problem with having 20 tests in a block?
There was a problem hiding this comment.
Not necessarily a problem but I find it easier to navigate/update a test file when tests are grouped. Looking at this file, we could group file picking and field picking into its own blocks. Though, of course, no strong preference, I'm happy to leave it as is if preferred.
extension/src/pipeline/quickPick.ts
Outdated
|
|
||
| if (yValues.length !== xFieldsLength) { | ||
| void Toast.showError( | ||
| 'The amount of y metrics needs to match the amount of x metrics. See [an example](https://dvc.org/doc/user-guide/project-structure/dvcyaml-files#available-configuration-fields) of a plot with multiple x metrics.' |
There was a problem hiding this comment.
[C] When there are multiple x metrics selected there must be an equal number of y metrics.
[Q] Also, did you consider using a version of quickPickLimitedValues when there are multiple xs? That would avoid the need for this validation. Thinking out loud => maybe we could have a quick pick that validates the number of items selected in the onDidAccept callback?
There was a problem hiding this comment.
[F] The CLI error is WARNING: In 'workspace', cannot have different number of x and y data sources. found 2 x and 4 y data sources.
There was a problem hiding this comment.
Also, did you consider using a version of quickPickLimitedValues when there are multiple xs? That would avoid the need for this validation. Thinking out loud => maybe we could have a quick pick that validates the number of items selected in the onDidAccept callback?
That might work! I didn't think we could control that in the onDidAccept but I'll try it out! Even if it doesn't work, we could still use quickPickLimitedValues to at least help with making sure the user doesn't select too many items 🤔
…dling-for-plot-wizard-field-dict
|
Going to merge and improve validation for the y metrics quick pick (#4798 (comment)) in a followup pr! |
julieg18
merged commit 7033ec6
into
allow-plot-wizard-multi-x-selection
3/3
main<- #4787 <- #4797 <- thisDemo
Check for only metric per file
Screen.Recording.2023-10-11.at.5.36.13.PM.mov
Check for x and y having the same amount of metrics
Screen.Recording.2023-10-11.at.5.37.00.PM.mov
Part of #4654