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

Fixing dependency generation for jinja templating engine. #2501

Merged
merged 2 commits into from Sep 19, 2016

Conversation

felixfontein
Copy link
Contributor

@felixfontein felixfontein commented Sep 19, 2016

Jinja template dependencies sometimes miss the main template itself, depending on whether the dependencies where stored in the internal cache in JinjaTemplates.get_string_deps or in JinjaTemplates.template_deps.

@@ -112,7 +112,7 @@ def get_string_deps(self, text):
filename = self.lookup.loader.get_source(self.lookup, dep_name)[1]
deps.add(filename)
sub_deps = self.get_deps(filename)
self.dependency_cache[dep_name] = sub_deps
self.dependency_cache[dep_name] = [filename] + sub_deps
Copy link
Member

@Kwpolska Kwpolska Sep 19, 2016

Choose a reason for hiding this comment

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

Are you sure this is the place? We return deps from this function, and it’s not taken from the cache. Perhaps add [filename] a line earlier?

Copy link
Contributor Author

@felixfontein felixfontein Sep 19, 2016

Choose a reason for hiding this comment

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

In that case, the line deps.add(filename) can go as well.

Copy link
Member

@Kwpolska Kwpolska Sep 19, 2016

Choose a reason for hiding this comment

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

Would be simpler to just modify sub_deps right when we define it, wouldn’t it?

Copy link
Contributor Author

@felixfontein felixfontein Sep 19, 2016

Choose a reason for hiding this comment

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

Check the current commit, is it what you want?

Copy link
Member

@Kwpolska Kwpolska Sep 19, 2016

Choose a reason for hiding this comment

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

Perfect, thanks. Approving and merging.

@Kwpolska Kwpolska merged commit fad241e into master Sep 19, 2016
@Kwpolska Kwpolska deleted the fix-jinja-deps branch Sep 19, 2016
@felixfontein
Copy link
Contributor Author

felixfontein commented Sep 19, 2016

Thanks for merging!

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

Successfully merging this pull request may close these issues.

None yet

2 participants