-
Notifications
You must be signed in to change notification settings - Fork 28
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove check for stages in plot wizard #4904
Changes from all commits
215bf04
ae70d2d
32b2de9
710519b
ecbfd98
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -262,6 +262,7 @@ const getPlotsYaml = (plotObj: PlotConfigData, indentSearchLines: string[]) => { | |
|
||
export const addPlotToDvcYamlFile = (cwd: string, plotObj: PlotConfigData) => { | ||
const dvcYamlFile = `${cwd}/dvc.yaml` | ||
ensureFileSync(dvcYamlFile) | ||
const dvcYamlDoc = loadYamlAsDoc(dvcYamlFile) | ||
|
||
if (!dvcYamlDoc) { | ||
|
@@ -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 | ||
|
||
void openFileInEditor(dvcYamlFile) | ||
return writeFileSync(dvcYamlFile, dvcYamlLines.join('\n')) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is an empty line on top of the dvc.yaml in the case of there being an empty file:
If we wanted to get rid of the top line, we could need to add an extra |
||
} | ||
|
||
const plotsEndPos = lineCounter.linePos(plots.range[2]).line | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -83,28 +83,8 @@ export class Pipeline extends DeferredDisposable { | |
} | ||
|
||
public async getCwd() { | ||
const focusedPipeline = this.getFocusedPipeline() | ||
if (focusedPipeline) { | ||
return focusedPipeline | ||
} | ||
|
||
await this.checkOrAddPipeline() | ||
|
||
const pipelines = this.model.getPipelines() | ||
if (!pipelines?.size) { | ||
return | ||
} | ||
if (pipelines.has(this.dvcRoot)) { | ||
return this.dvcRoot | ||
} | ||
if (pipelines.size === 1) { | ||
return [...pipelines][0] | ||
} | ||
|
||
return quickPickOne( | ||
[...pipelines], | ||
'Select a Pipeline to Run Command Against' | ||
) | ||
return this.findPipelineCwd() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
} | ||
|
||
public async checkOrAddPipeline() { | ||
|
@@ -158,11 +138,7 @@ export class Pipeline extends DeferredDisposable { | |
} | ||
|
||
public async addDataSeriesPlot() { | ||
const cwd = await this.getCwd() | ||
|
||
if (!cwd) { | ||
return | ||
} | ||
const cwd = (await this.findPipelineCwd()) || this.dvcRoot | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We now default to |
||
|
||
const plotConfiguration = await pickPlotConfiguration(cwd) | ||
|
||
|
@@ -177,6 +153,29 @@ export class Pipeline extends DeferredDisposable { | |
return appendFileSync(join(this.dvcRoot, TEMP_DAG_FILE), '\n') | ||
} | ||
|
||
private findPipelineCwd() { | ||
const focusedPipeline = this.getFocusedPipeline() | ||
if (focusedPipeline) { | ||
return focusedPipeline | ||
} | ||
|
||
const pipelines = this.model.getPipelines() | ||
if (!pipelines?.size) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Out of scope, but since we have a lint rule to test zero length explicitly ( There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Makes sense to me. I can look into a eslint rule for it! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks like the lint rule we have ( |
||
return | ||
} | ||
if (pipelines.has(this.dvcRoot)) { | ||
return this.dvcRoot | ||
} | ||
if (pipelines.size === 1) { | ||
return [...pipelines][0] | ||
} | ||
|
||
return quickPickOne( | ||
[...pipelines], | ||
'Select a Pipeline to Run Command Against' | ||
) | ||
} | ||
|
||
private async initialize() { | ||
this.dispose.track( | ||
this.data.onDidUpdate(({ dag, stages }) => { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.