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

plots: Fix repo with removed plots #7643

Merged
merged 1 commit into from
Apr 27, 2022
Merged

Conversation

daavoo
Copy link
Contributor

@daavoo daavoo commented Apr 27, 2022

Changes in dvc-render 0.0.5 caused an error.

In iterative/dvc-render#24 the behavior for get_filled_template changed to return an empty string. Previously it returned a valid dictionary with empty data field.

Updated to_json logic in dvc to not call get_filled_template at all if there are no datapoints.

@daavoo daavoo requested a review from pared April 27, 2022 09:17
@daavoo daavoo requested a review from a team as a code owner April 27, 2022 09:17
@daavoo daavoo added product: VSCode Integration with VSCode extension bugfix fixes bug release-blocker Blocks a release labels Apr 27, 2022
Comment on lines -245 to -246
call(capsys)
call(capsys, subcommand="diff")
Copy link
Contributor Author

@daavoo daavoo Apr 27, 2022

Choose a reason for hiding this comment

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

@pared we only checked that this didn't raise an error but not the expected result. Updating to dvc-render 0.0.5 raises an error.

I updated to_json to return an empty list for vega plots to match the image plots behavior.

If this is not the expected return format, we need to remove:

https://github.com/iterative/dvc-render/blob/369396f9e833e9b82724b21d0c5f1b853b156d06/src/dvc_render/vega.py#L46-L47

In order to get back to previous behavior. Before iterative/dvc-render#24 , this tests were passing but the returns for vega plots were a vega template without data.

Changes in dvc-render 0.0.5 caused error.
@daavoo daavoo force-pushed the integration-test-removed-pots branch from 2edaf67 to ec3630d Compare April 27, 2022 09:26
Copy link
Contributor

@pared pared left a comment

Choose a reason for hiding this comment

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

LGTM.
As discussed off this thread - I agree with @daavoo that we should not reach to_json if there is no data to display. Lets create issue for that?

@daavoo daavoo enabled auto-merge (rebase) April 27, 2022 09:41
@daavoo daavoo merged commit bea7d56 into main Apr 27, 2022
@daavoo daavoo deleted the integration-test-removed-pots branch April 27, 2022 09:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix fixes bug product: VSCode Integration with VSCode extension release-blocker Blocks a release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants