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: Pass templates_dir to match_renderers. #7820

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion dvc/commands/plots.py
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,9 @@ def run(self):
out if self.args.json else os.path.join(out, "static")
)
renderers = match_renderers(
plots_data=plots_data, out=renderers_out
plots_data=plots_data,
out=renderers_out,
templates_dir=self.repo.plots.templates_dir,
Comment on lines 77 to +80
Copy link
Member

Choose a reason for hiding this comment

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

Also, it'd be better to push more of this logic inside the API so that you can test them easily.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for my lack of understanding, could you elaborate on what does "the API" refers to?

)

if self.args.show_vega:
Expand Down
2 changes: 1 addition & 1 deletion dvc/render/match.py
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ def match_renderers(
if out is not None:
plot_properties["out"] = out
if templates_dir is not None:
plot_properties["templates_dir"] = templates_dir
plot_properties["template_dir"] = templates_dir
datapoints, plot_properties = to_datapoints(
renderer_class, group, plot_properties
)
Expand Down
3 changes: 2 additions & 1 deletion dvc/repo/plots/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -228,7 +228,8 @@ def modify(self, path, props=None, unset=None):

@cached_property
def templates_dir(self):
return os.path.join(self.repo.dvc_dir, "plots")
if self.repo.dvc_dir:
return os.path.join(self.repo.dvc_dir, "plots")


def _is_plot(out: "Output") -> bool:
Expand Down
30 changes: 30 additions & 0 deletions tests/unit/command/test_plots.py
Original file line number Diff line number Diff line change
Expand Up @@ -240,6 +240,36 @@ def test_plots_path_is_quoted_and_resolved_properly(
assert expected_url in out


def test_should_pass_template_dir(tmp_dir, dvc, mocker, capsys):
Copy link
Member

@skshetry skshetry May 27, 2022

Choose a reason for hiding this comment

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

Both of these tests are not useful. I'll suggest testing actual behaviour instead if you can or get rid of them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Both of these tests are not useful.

@skshetry Could you elaborate on why are not useful?

I'll suggest testing actual behaviour instead if you can or get rid of them.

Behavior is tested in https://github.com/iterative/dvc-render/blob/main/tests/test_templates.py#L39

cli_args = parse_args(
[
"plots",
"diff",
"HEAD~1",
"--json",
"--targets",
"plot.csv",
]
)
cmd = cli_args.func(cli_args)

data = mocker.MagicMock()
mocker.patch("dvc.repo.plots.diff.diff", return_value=data)

renderers = mocker.MagicMock()
match_renderers = mocker.patch(
"dvc.render.match.match_renderers", return_value=renderers
)

assert cmd.run() == 0

match_renderers.assert_called_once_with(
plots_data=data,
out="dvc_plots",
templates_dir=str(tmp_dir / ".dvc/plots"),
)


@pytest.mark.parametrize(
"output", ("some_out", os.path.join("to", "subdir"), None)
)
Expand Down
15 changes: 15 additions & 0 deletions tests/unit/render/test_match.py
Original file line number Diff line number Diff line change
Expand Up @@ -178,3 +178,18 @@ def test_match_renderers_with_out(tmp_dir, mocker):
assert (
tmp_dir / "foo" / "workspace_other_file.jpg"
).read_bytes() == b"content2"


def test_match_renderers_template_dir(mocker):
from dvc_render import vega

vega_render = mocker.spy(vega.VegaRenderer, "__init__")
data = {
"v1": {
"data": {"file.json": {"data": [{"y": 4}, {"y": 5}], "props": {}}}
},
}

match_renderers(data, templates_dir="foo")

assert vega_render.call_args[1]["template_dir"] == "foo"