Skip to content

Render optional attributes with default values as mandatory #909

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.

Sign up for GitHub

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

Open
ogenstad wants to merge 1 commit into
base: stable
Choose a base branch
from
+54 −2
Open

Render optional attributes with default values as mandatory #909

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
1 change: 1 addition & 0 deletions changelog/894.fixed.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Generate protocols where optional attributes with a default value renders attributes that are required, i.e. not nullable.
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Apr 2, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Tighten changelog wording for clarity and grammar.

Line 1 is grammatically awkward and a bit ambiguous. Consider a clearer statement focused on generated typing behavior.

Suggested wording 
-Generate protocols where optional attributes with a default value renders attributes that are required, i.e. not nullable.
+Generate protocols so attributes with a default value are rendered as required (not nullable).
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
Generate protocols where optional attributes with a default value renders attributes that are required, i.e. not nullable.
Generate protocols so attributes with a default value are rendered as required (not nullable).
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@changelog/894.fixed.md` at line 1, Rewrite the single-line changelog to be
clearer and grammatically correct by stating the behavior precisely; replace the
current sentence with something like: "Generate protocols where optional
attributes with a default value are rendered as required (non-nullable)
attributes," ensuring it mentions "optional attributes with a default value" and
that they become "required (non-nullable)" in generated typing; update the line
in changelog/894.fixed.md accordingly.

Copy link
Copy Markdown
Contributor

@ajtmccarty ajtmccarty Apr 2, 2026

Choose a reason for hiding this comment

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

this suggestion is easier to understand, but also leaves out the word "optional"

Copy link
Copy Markdown

@coderabbitai coderabbitai bot Apr 2, 2026

Choose a reason for hiding this comment

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

@ajtmccarty, that's a fair point — "optional" is meaningful here since the fix specifically targets attributes marked as optional in the schema that also have a default value. Here's a revised suggestion that preserves it:

-Generate protocols where optional attributes with a default value renders attributes that are required, i.e. not nullable.
+Generate protocols so that optional attributes with a default value are rendered as required (not nullable).

This keeps "optional attributes with a default value" intact while fixing the grammatical awkwardness of the original.

2 changes: 1 addition & 1 deletion infrahub_sdk/protocols_generator/generator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ def _jinja2_filter_syncify(value: str | list, sync: bool = False) -> str | list:
def _jinja2_filter_render_attribute(value: AttributeSchemaAPI) -> str:
attribute_kind: str = ATTRIBUTE_KIND_MAP[value.kind]

if value.optional:
if value.optional and value.default_value is None:
attribute_kind += "Optional"

return f"{value.name}: {attribute_kind}"
Expand Down
53 changes: 52 additions & 1 deletion tests/unit/sdk/test_protocols_generator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
from dataclasses import dataclass
from typing import TYPE_CHECKING
from typing import TYPE_CHECKING, Any

import pytest

from infrahub_sdk import InfrahubClient
from infrahub_sdk.protocols_generator.generator import CodeGenerator
from infrahub_sdk.schema import AttributeSchemaAPI

if TYPE_CHECKING:
from pytest_httpx import HTTPXMock
Expand Down Expand Up @@ -36,6 +37,56 @@ class SyncifyTestCase:
]


@dataclass
class RenderAttributeTestCase:
name: str
optional: bool
default_value: Any
expected: str


RENDER_ATTRIBUTE_TEST_CASES = [
RenderAttributeTestCase(
name="required-no-default",
optional=False,
default_value=None,
expected="enabled: Boolean",
),
RenderAttributeTestCase(
name="required-with-default",
optional=False,
default_value=True,
expected="enabled: Boolean",
),
RenderAttributeTestCase(
name="optional-no-default",
optional=True,
default_value=None,
expected="enabled: BooleanOptional",
),
RenderAttributeTestCase(
name="optional-with-default",
optional=True,
default_value=True,
expected="enabled: Boolean",
),
]


@pytest.mark.parametrize(
"test_case",
[pytest.param(tc, id=tc.name) for tc in RENDER_ATTRIBUTE_TEST_CASES],
)
async def test_filter_render_attribute(test_case: RenderAttributeTestCase) -> None:
attr = AttributeSchemaAPI(
name="enabled",
kind="Boolean",
optional=test_case.optional,
default_value=test_case.default_value,
)
assert CodeGenerator._jinja2_filter_render_attribute(attr) == test_case.expected


@pytest.mark.parametrize(
"test_case",
[pytest.param(tc, id=tc.name) for tc in SYNCIFY_TEST_CASES],
Expand Down