Skip to content

fix:boolean parameter without defaullt value defaults to False #352

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
DimitriKwihangana wants to merge 4 commits into
base: main
Choose a base branch
from
Open

fix:boolean parameter without defaullt value defaults to False #352

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
83e9e39
fix:boolean parameter without defaullt value defaults to False
DimitriKwihangana Feb 2, 2026
569d5e4
added self required so that optional boolean parameters can remain None
DimitriKwihangana Feb 2, 2026
4c34c8d
fix: merge normalized_value if blocks and unit tests
DimitriKwihangana Feb 4, 2026
fd8cd93
resolving CI failure
DimitriKwihangana Feb 4, 2026
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
8 changes: 5 additions & 3 deletions openhexa/sdk/pipelines/parameter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -526,10 +526,12 @@ def _validate_single(self, value: typing.Any):
normalized_value = self.default

Copy link
Contributor

@yolanfery yolanfery Feb 3, 2026

Choose a reason for hiding this comment

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

I propose to add a unit test to cover this edge case

Copy link
Contributor

@yolanfery yolanfery Feb 3, 2026

Choose a reason for hiding this comment

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

(This ensures that we avoid regressions in the future but also it greatly simplifies code reviews, if the test coverage is good and the intent of tests are good we might not need to pull up the code and test)

Copy link
Contributor

@bramj bramj Feb 4, 2026

Choose a reason for hiding this comment

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

Agreed

if normalized_value is None:
if self.required:
if isinstance(self.type, Boolean):
normalized_value = False
elif self.required:
raise ParameterValueError(f"{self.code} is required")

return None
else:
return None

pre_validated = self.type.validate(normalized_value)
if self.choices is not None and pre_validated not in self.choices:
Expand Down
8 changes: 6 additions & 2 deletions tests/test_parameter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -287,9 +287,13 @@ def test_parameter_validate_single():
parameter_2 = Parameter("arg2", type=int, default=3)
assert parameter_2.validate(None) == 3

# not required, no default
# not required, no default - booleans default to False
parameter_3 = Parameter("arg3", type=bool, required=False)
assert parameter_3.validate(None) is None
assert parameter_3.validate(None) is False

# required, no default - booleans default to False
parameter_5 = Parameter("arg5", type=bool, required=True)
assert parameter_5.validate(None) is False

# choices
parameter_4 = Parameter("arg4", type=str, choices=["ab", "cd"])
Expand Down
Loading