Skip to content

Conversation

@pared
Copy link
Contributor

@pared pared commented May 20, 2020

  • ❗ I have followed the Contributing to DVC checklist.

  • πŸ“– If this PR requires documentation updates, I have created a separate PR (or issue, at least) in dvc.org and linked it here. If the CLI API is changed, I have updated tab completion scripts.

  • ❌ I will check DeepSource, CodeClimate, and other sanity checks below. (We consider them recommendatory and don't expect everything to be addressed. Please fix things that actually improve code or fix bugs.)

Thank you for the contribution - we'll try to review it as soon as possible. πŸ™
Fixes #3755

@pared pared requested a review from jorgeorpinel May 20, 2020 12:23

This comment was marked as outdated.

Copy link
Contributor

Choose a reason for hiding this comment

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

I found this https://github.com/vega/schema are templates basically this? Vega schema files. If so, I suggest

Suggested change
"Vega specification file to inject with the data. "
"Vega JSON schema file to inject with the data. "

and will update the docs to link to this repo for reference.

Copy link
Contributor Author

@pared pared May 21, 2020

Choose a reason for hiding this comment

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

Hmm actually template is an Vega JSON schema with DVC placeholders (anchors) that will be filled with proper values after using dvc plots. Also, template does not have to be JSON at all,
that is bad on my side. If user wants it can be HTML as well, the only thing that makes it template is existence of DVC_METRIC_DATA anchor inside.

Also this might change depending on discussion under #3818

So I think that as of now, the best description would be to leave it as is (File to be injected with data. See {link}

Copy link
Contributor

Choose a reason for hiding this comment

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

OK np let's just come up with a general description here and we'll have to revisit the docs to explain this a little better. The anchors are listed but it's not clear that templates are a combo of Vega schema and DVC anchors + possibly HTML. TBH I still don't totally get it haha. But how about

Suggested change
"Vega specification file to inject with the data. "
"Special JSON or HTML schema to inject with the data. "

?

Also this might change depending on discussion under #3818

No worries. We can revisit this and docs if/when that happens.

Copy link
Contributor

Choose a reason for hiding this comment

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

p.s. added checkbox to improve plot template desc. to #1175.

@pared pared force-pushed the 3755_link_to_docs branch from 6e71f84 to 8311905 Compare May 21, 2020 16:47
@pared pared requested a review from jorgeorpinel May 21, 2020 16:47
Copy link
Contributor

@jorgeorpinel jorgeorpinel left a comment

Choose a reason for hiding this comment

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

Forgot a little detail but yes, looks good. Thanks!

Co-authored-by: Jorge Orpinel <jorgeorpinel@users.noreply.github.com>
@efiop efiop merged commit 61f30ed into treeverse:master May 21, 2020
@pared pared mentioned this pull request May 22, 2020
3 tasks
@pared pared deleted the 3755_link_to_docs 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

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Plot: link to docs

3 participants