chore(generator): use global var for autopopulated fields#17890
chore(generator): use global var for autopopulated fields#17890hebaalazzeh wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors several Jinja2 templates to pre-calculate whether a service has auto-populated fields, replacing complex inline filters with a simpler boolean variable. The review feedback correctly points out that defining variables and executing loops before the {% extends %} tag violates Jinja2 standards, which can lead to syntax errors or scoping issues. It is recommended to move these variable definitions and loops inside the {% block content %} block to ensure correct rendering and compliance.
| {% set has_auto_populated_fields = namespace(value=false) %} | ||
| {% for method in service.methods.values() %} | ||
| {% set method_settings = api.all_method_settings.get(method.meta.address.proto) %} | ||
| {% if method_settings and method_settings.auto_populated_fields %} | ||
| {% set has_auto_populated_fields.value = true %} | ||
| {% endif %} | ||
| {% endfor %} | ||
| {% extends '_base.py.j2' %} | ||
|
|
||
| {% block content %} |
There was a problem hiding this comment.
In Jinja2, the {% extends %} tag should be the first tag in the template. Defining variables and executing loops before {% extends %} can lead to syntax errors or unexpected rendering behavior in some Jinja2 environments. Additionally, variables defined outside of blocks in a child template are often not accessible inside the blocks. Moving the variable definition and loop inside the {% block content %} block ensures compliance with Jinja2 standards and guarantees correct scoping.
{% extends '_base.py.j2' %}
{% block content %}
{% set has_auto_populated_fields = namespace(value=false) %}
{% for method in service.methods.values() %}
{% set method_settings = api.all_method_settings.get(method.meta.address.proto) %}
{% if method_settings and method_settings.auto_populated_fields %}
{% set has_auto_populated_fields.value = true %}
{% endif %}
{% endfor %}
| {% set has_auto_populated_fields = namespace(value=false) %} | ||
| {% for method in service.methods.values() %} | ||
| {% set method_settings = api.all_method_settings.get(method.meta.address.proto) %} | ||
| {% if method_settings and method_settings.auto_populated_fields %} | ||
| {% set has_auto_populated_fields.value = true %} | ||
| {% endif %} | ||
| {% endfor %} | ||
| {% extends "_base.py.j2" %} | ||
|
|
||
| {% block content %} |
There was a problem hiding this comment.
In Jinja2, the {% extends %} tag should be the first tag in the template. Defining variables and executing loops before {% extends %} can lead to syntax errors or unexpected rendering behavior in some Jinja2 environments. Additionally, variables defined outside of blocks in a child template are often not accessible inside the blocks. Moving the variable definition and loop inside the {% block content %} block ensures compliance with Jinja2 standards and guarantees correct scoping.
{% extends "_base.py.j2" %}
{% block content %}
{% set has_auto_populated_fields = namespace(value=false) %}
{% for method in service.methods.values() %}
{% set method_settings = api.all_method_settings.get(method.meta.address.proto) %}
{% if method_settings and method_settings.auto_populated_fields %}
{% set has_auto_populated_fields.value = true %}
{% endif %}
{% endfor %}
| {# TODO(https://github.com/googleapis/gapic-generator-python/issues/2121): Remove the following variable (and the condition later in this file) for async rest transport once support for it is GA. #} | ||
| {% set rest_async_io_enabled = api.all_library_settings[api.naming.proto_package].python_settings.experimental_features.rest_async_io_enabled %} | ||
| {% set has_auto_populated_fields = namespace(value=false) %} | ||
| {% for method in service.methods.values() %} | ||
| {% set method_settings = api.all_method_settings.get(method.meta.address.proto) %} | ||
| {% if method_settings and method_settings.auto_populated_fields %} | ||
| {% set has_auto_populated_fields.value = true %} | ||
| {% endif %} | ||
| {% endfor %} | ||
| {% extends '_base.py.j2' %} | ||
|
|
||
| {% block content %} |
There was a problem hiding this comment.
In Jinja2, the {% extends %} tag should be the first tag in the template. Defining variables and executing loops before {% extends %} can lead to syntax errors or unexpected rendering behavior in some Jinja2 environments. Additionally, variables defined outside of blocks in a child template are often not accessible inside the blocks. Moving the variable definition and loop inside the {% block content %} block ensures compliance with Jinja2 standards and guarantees correct scoping.
{# TODO(https://github.com/googleapis/gapic-generator-python/issues/2121): Remove the following variable (and the condition later in this file) for async rest transport once support for it is GA. #}
{% set rest_async_io_enabled = api.all_library_settings[api.naming.proto_package].python_settings.experimental_features.rest_async_io_enabled %}
{% extends '_base.py.j2' %}
{% block content %}
{% set has_auto_populated_fields = namespace(value=false) %}
{% for method in service.methods.values() %}
{% set method_settings = api.all_method_settings.get(method.meta.address.proto) %}
{% if method_settings and method_settings.auto_populated_fields %}
{% set has_auto_populated_fields.value = true %}
{% endif %}
{% endfor %}
| {# TODO(https://github.com/googleapis/gapic-generator-python/issues/2121): Remove the following variable (and the condition later in this file) for async rest transport once support for it is GA. #} | ||
| {% set rest_async_io_enabled = api.all_library_settings[api.naming.proto_package].python_settings.experimental_features.rest_async_io_enabled %} | ||
| {% set has_auto_populated_fields = namespace(value=false) %} | ||
| {% for method in service.methods.values() %} | ||
| {% set method_settings = api.all_method_settings.get(method.meta.address.proto) %} | ||
| {% if method_settings and method_settings.auto_populated_fields %} | ||
| {% set has_auto_populated_fields.value = true %} | ||
| {% endif %} | ||
| {% endfor %} | ||
| {% extends "_base.py.j2" %} | ||
|
|
||
| {% block content %} |
There was a problem hiding this comment.
In Jinja2, the {% extends %} tag should be the first tag in the template. Defining variables and executing loops before {% extends %} can lead to syntax errors or unexpected rendering behavior in some Jinja2 environments. Additionally, variables defined outside of blocks in a child template are often not accessible inside the blocks. Moving the variable definition and loop inside the {% block content %} block ensures compliance with Jinja2 standards and guarantees correct scoping.
{# TODO(https://github.com/googleapis/gapic-generator-python/issues/2121): Remove the following variable (and the condition later in this file) for async rest transport once support for it is GA. #}
{% set rest_async_io_enabled = api.all_library_settings[api.naming.proto_package].python_settings.experimental_features.rest_async_io_enabled %}
{% extends "_base.py.j2" %}
{% block content %}
{% set has_auto_populated_fields = namespace(value=false) %}
{% for method in service.methods.values() %}
{% set method_settings = api.all_method_settings.get(method.meta.address.proto) %}
{% if method_settings and method_settings.auto_populated_fields %}
{% set has_auto_populated_fields.value = true %}
{% endif %}
{% endfor %}
16f3b08 to
f7a31e6
Compare
Updating the use of global variable for auto populated fields