Skip to content

model: use is None instead of falsy check in check_default_min_max - #517

Merged
sschleemilch merged 2 commits into
COVESA:masterfrom
SoundMatt:fix/check-default-min-max-falsy
May 26, 2026
Merged

model: use is None instead of falsy check in check_default_min_max#517
sschleemilch merged 2 commits into
COVESA:masterfrom
SoundMatt:fix/check-default-min-max-falsy

Conversation

@SoundMatt

Copy link
Copy Markdown
Contributor

Problem

check_default_min_max in src/vss_tools/model.py returned early
when not self.default was truthy. Python treats 0, False, "",
and [] as falsy, so a numeric signal like:

Vehicle.SomeSignal:
  type: sensor
  datatype: uint8
  description: "..."
  min: 10
  default: 0   # < min, should be rejected — but silently passed

…silently passed validation. Same for default: 0 with a negative
max. The intended range check was a no-op for any falsy default.

Fix

-        if not self.default:
+        if self.default is None:

The downstream loop already handles list-vs-scalar correctly; with
this fix, default=0 runs through the comparison and raises
ValidationError when out of range.

Test

Three new parametrize cases in tests/test_model.py:

({"datatype": "uint8", "min": 1, "default": 0}, False),   # bug case
({"datatype": "int8",  "max": -1, "default": 0}, False),  # bug case
({"datatype": "uint8", "min": 0, "default": 0}, True),    # sanity

The first two would have passed (incorrectly) before this PR. The
third confirms default=0 is still allowed when within bounds.

Notes

Comment thread src/vss_tools/model.py Outdated

def check_default_min_max(self) -> Self:
if not self.default:
# Use an explicit None check rather than a falsy check: `not self.default`

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

That's pretty verbose. Just fix the if statement, which is a good catch

Comment thread tests/test_model.py Outdated
({"datatype": "uint8", "max": 100, "default": 90}, True),
({"datatype": "uint8", "min": 10, "default": 5}, False),
({"datatype": "uint8", "min": 10, "default": 10}, True),
# Regression: `default=0` (a falsy but valid numeric default) used to

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

No need for those comments. Just add the tests

@erikbosch

Copy link
Copy Markdown
Collaborator

MoM:

  • Presented at meeting
  • ok to merge after maintainer approval

@erikbosch

Copy link
Copy Markdown
Collaborator

@SoundMatt - will you take a look at the comments from Sebastian

@SoundMatt

Copy link
Copy Markdown
Contributor Author

Addressed — verbose comments removed.

SoundMatt added 2 commits May 22, 2026 14:39
`check_default_min_max` returned early when `not self.default` was
truthy. Python treats 0, False, "", and [] as falsy, so a numeric
signal with `default=0` and a positive `min` (or default=0 and a
negative `max`) silently bypassed the range check. The validation
intended to catch invalid defaults was a no-op for those values.

Switch to an explicit `self.default is None` check. The downstream
loop already handles list/scalar shapes correctly; with this fix,
default=0 is run through the comparison and will trigger
ValidationError when out of range.

Adds three regression cases to tests/test_model.py covering
default=0 with various min/max bounds (the primary bug case, plus
a sanity case for default=0 within bounds).

Signed-off-by: Matt Jones <47545907+SoundMatt@users.noreply.github.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Signed-off-by: Matt Jones <47545907+SoundMatt@users.noreply.github.com>
@SoundMatt
SoundMatt force-pushed the fix/check-default-min-max-falsy branch from 74a66f8 to dea2b3f Compare May 22, 2026 21:39
@erikbosch

Copy link
Copy Markdown
Collaborator

@sschleemilch - do you want to take a second look here

@sschleemilch
sschleemilch merged commit a5432d6 into COVESA:master May 26, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants