Remove check for stages in plot wizard #4904
Conversation
| @@ -276,8 +277,9 @@ export const addPlotToDvcYamlFile = (cwd: string, plotObj: PlotConfigData) => { | |||
| if (!plots?.range) { | |||
| const plotYaml = getPlotsYaml(plotObj, dvcYamlLines) | |||
| dvcYamlLines.push(...plotYaml) | |||
| writeFileSync(dvcYamlFile, dvcYamlLines.join('\n')) | |||
| return | |||
There was a problem hiding this comment.
This "no plots found" route is actually missing the openFileInEditor line. I've added it and updated the tests to check for this function being called.
| return | ||
|
|
||
| void openFileInEditor(dvcYamlFile) | ||
| return writeFileSync(dvcYamlFile, dvcYamlLines.join('\n')) |
There was a problem hiding this comment.
There is an empty line on top of the dvc.yaml in the case of there being an empty file:
plots:
- step vs acc:
template: confusion
x: step
y:
training/plots/metrics/train/acc.json: acc
If we wanted to get rid of the top line, we could need to add an extra if check. I decided to just keep it to avoid extra complexity.
| if (!cwd) { | ||
| return | ||
| } | ||
| const cwd = (await this.findPipelineCwd()) || this.dvcRoot |
There was a problem hiding this comment.
We now default to this.dvcRoot if a cwd isn't found.
| } | ||
|
|
||
| const pipelines = this.model.getPipelines() | ||
| if (!pipelines?.size) { |
There was a problem hiding this comment.
Out of scope, but since we have a lint rule to test zero length explicitly (something.length === 0 instead of !something.length). Should we have a similar rule for size as well?
There was a problem hiding this comment.
Makes sense to me. I can look into a eslint rule for it!
There was a problem hiding this comment.
Looks like the lint rule we have (explicit-link-check) works for both arrays and size. This is allowed though since we have an undefined check there as well.
|
Code Climate has analyzed commit ecbfd98 and detected 0 issues on this pull request. The test coverage on the diff in this pull request is 93.7% (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. |
| [...pipelines], | ||
| 'Select a Pipeline to Run Command Against' | ||
| ) | ||
| return this.findPipelineCwd() |
There was a problem hiding this comment.
Going to mention it here since it was discussed asynchronously with Matt, but it would simplify things to just have the Plot Wizard automatically create a dvc.yaml in the root vs looking for one elsewhere.
While this means that it wouldn't work for people with workspaces or nested dvc.yamls, users who do those are most likely advanced DVC users who don't have a need for the plot wizard.
I chose to keep it like it is for now but I'm happy to update if preferred.
Demo
Screen.Recording.2023-10-26.at.8.01.02.AM.mov