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

template lookup: restore variables between calls #55126

Open
wants to merge 2 commits into
base: devel
from

Conversation

Projects
None yet
3 participants
@mkrizek
Copy link
Contributor

commented Apr 11, 2019

SUMMARY

Fixes #55113

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

template lookup

@@ -107,6 +107,9 @@ def run(self, terms, variables, **kwargs):
# do the templating
res = self._templar.template(template_data, preserve_trailing_newlines=True,
convert_data=convert_data_p, escape_backslashes=False)
# restore old variables
self._templar.set_available_variables(variables)

This comment has been minimized.

Copy link
@bcoca

bcoca Apr 11, 2019

Member

i would push this to the top of the loop, probably first thing after 'if'

This comment has been minimized.

Copy link
@mkrizek

mkrizek Apr 12, 2019

Author Contributor

How would that work? We are setting lookup file/template specific vars on line 105, before templating happens. So the restore needs to happen right after templating.

This comment has been minimized.

Copy link
@bcoca

bcoca Apr 15, 2019

Member

Sorry, i was unclear, I didn't mean just this line, meant the vars copy/update block .

Revisiting this line, I would actually put outside loop, after it ends.

Also the existing variables.copy() will bypass your efforts as 'container types' will copy by ref, making modifications persist across not only invocations but in returned vars. see deepcopy or deepish_copy.

This comment has been minimized.

Copy link
@mkrizek

mkrizek Apr 16, 2019

Author Contributor

Sorry, i was unclear, I didn't mean just this line, meant the vars copy/update block .

I don't follow, what would be the gain if we moved the vars copy/update block to the top of the loop?

Revisiting this line, I would actually put outside loop, after it ends.

Also the existing variables.copy() will bypass your efforts as 'container types' will copy by ref, making modifications persist across not only invocations but in returned vars. see deepcopy or deepish_copy.

Good points, fixed both.

This comment has been minimized.

Copy link
@bcoca

bcoca Apr 16, 2019

Member

the moving is more stylistic (prep on top), so you can ignore

@mkrizek mkrizek requested a review from bcoca Apr 15, 2019

@@ -67,6 +68,8 @@ def run(self, terms, variables, **kwargs):
variable_start_string = kwargs.get('variable_start_string', None)
variable_end_string = kwargs.get('variable_end_string', None)

old_vars = self._templar._available_variables

This comment has been minimized.

Copy link
@bcoca

bcoca Apr 16, 2019

Member

I could swear i created a get_variables method to avoid using the internal dict directly, but cannot find it now ...

This comment has been minimized.

Copy link
@mkrizek

mkrizek Apr 16, 2019

Author Contributor

I can add it in a separate PR.

This comment has been minimized.

Copy link
@bcoca

bcoca Apr 18, 2019

Member

ah, was part of become plugins but others decided to drop that method ...

This comment has been minimized.

Copy link
@mkrizek

mkrizek Apr 18, 2019

Author Contributor

FWIW I created #55435.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.