Skip to content
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

add schema for top-level plots #19

Merged
merged 2 commits into from
Sep 14, 2022
Merged

add schema for top-level plots #19

merged 2 commits into from
Sep 14, 2022

Conversation

skshetry
Copy link
Member

@skshetry skshetry commented Sep 14, 2022

Closes #14.

@skshetry skshetry requested review from pared and daavoo September 14, 2022 12:27
@skshetry skshetry self-assigned this Sep 14, 2022
@daavoo daavoo linked an issue Sep 14, 2022 that may be closed by this pull request
@@ -60,6 +63,27 @@ class PlotFlags(OutFlags):
template: FilePath = Field(None, description="Default plot template")


class TopLevelPlotFlags(BaseModel):
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am duplicating here so that they: plots' outputs and the top-level plots are separate from the start and easier to iterate on in the future.

gen.py Outdated
x_label: str = Field(None, description="Default label for the x-axis")
y_label: str = Field(None, description="Default label for the y-axis")
title: str = Field(None, description="Default plot title")
template: FilePath = Field(None, description="Default plot template")
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Template can also be a string name of the template, but I guess since the FilePath is str it is ok for now? Or should we create a separate type for it?

Copy link
Member Author

@skshetry skshetry Sep 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean by string name of the template? Can it be anything other than a path to a file?

Or should we create a separate type for it?

It’s a separate type, i.e subclassed from str.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well it can be a string, for example, if user writes confusion we will search for the template that has DEFAULT_NAME field with this value. If it is not found, we will start checkif if it wasn't a path.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, thanks. Is that also the case for template in plots output?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even though the schema is going to be the same, I think it's better to be explicit about what types are supported in dvc.yaml file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Add top-level plots section
2 participants