Skip to content

[WIP] plots: html_template -> html#6221

Closed
pared wants to merge 1 commit into
treeverse:masterfrom
pared:polish_html
Closed

[WIP] plots: html_template -> html#6221
pared wants to merge 1 commit into
treeverse:masterfrom
pared:polish_html

Conversation

@pared
Copy link
Copy Markdown
Contributor

@pared pared commented Jun 24, 2021

Thank you for the contribution - we'll try to review it as soon as possible. 🙏

Adressing #5694 (review)

@pared pared requested a review from a team as a code owner June 24, 2021 08:59
@pared pared requested review from isidentical and jorgeorpinel and removed request for isidentical June 24, 2021 08:59
Comment thread dvc/config_schema.py
Optional("parametrization", default=True): Bool
},
"plots": {"html_template": str},
"plots": {"html": str},
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can it compatible with the old schema? Some users might have an old version in their workspace.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is true, but the feature is relatively new and I think there is more harm in leaving html_template because users might associate template with vega templates and not understand what it actually is.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we adopt the old version, and transfer it to new ones? Or at least give some friendly messages when it happens.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This feature was only introduced in the last minor version or so, and it was specifically developed for the Studio team, so I wouldn't expect many (probably any) other people are using it. Feel free to support both or whatever you think is best, but I don't think this is a breaking change worth worrying about.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

AFAIK @mnrozhkov is the person that needs to know about this change, also one user on discord. I will try to reach out to this user.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Studio guys will complain about problems with this, we should not break an already released schema. We have to keep older name, but we could introduce an alias.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

My mistake, I didn't think it would be a big deal for studio.

Copy link
Copy Markdown
Contributor

@jorgeorpinel jorgeorpinel Jul 6, 2021

Choose a reason for hiding this comment

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

Related feedback, for the record: #5694 (review) (for the alias)

@pared
Copy link
Copy Markdown
Contributor Author

pared commented Jun 30, 2021

@jorgeorpinel could you take a look if description change seems allright?

Comment thread dvc/command/plots.py
)
parser.add_argument(
"--html-template",
"--html",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is backwards incompatible, right? Can we keep an alias or something?

Copy link
Copy Markdown
Contributor

@efiop efiop left a comment

Choose a reason for hiding this comment

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

Comment thread dvc/command/plots.py
"--html",
default=None,
help="Custom HTML template for VEGA visualization.",
help="Custom HTML for VEGA visualization.",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
help="Custom HTML for VEGA visualization.",
help="File with custom HTML container for DVC plots.",

@efiop efiop changed the title plots: html_template -> html [WIP] plots: html_template -> html Jul 18, 2021
@efiop
Copy link
Copy Markdown
Contributor

efiop commented Oct 2, 2021

@pared What's the plan with this PR?

@efiop efiop added the awaiting response we are waiting for your reply, please respond! :) label Oct 2, 2021
@pared
Copy link
Copy Markdown
Contributor Author

pared commented Oct 4, 2021

@efiop

Seems like there are not too many reports of user confusion. I think we can close for now and get back to it if we actually get any requests for it.

@pared pared closed this Oct 4, 2021
@pared pared deleted the polish_html branch January 4, 2022 10:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

awaiting response we are waiting for your reply, please respond! :)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants