Make label, description, and value optional on all attribute constructors#3009
Make label, description, and value optional on all attribute constructors#3009fabiencastan wants to merge 3 commits intoalicevision:developfrom
Conversation
… label from name Co-authored-by: fabiencastan <153585+fabiencastan@users.noreply.github.com>
…ault Co-authored-by: fabiencastan <153585+fabiencastan@users.noreply.github.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3009 +/- ##
===========================================
+ Coverage 81.99% 82.06% +0.07%
===========================================
Files 69 69
Lines 9315 9396 +81
===========================================
+ Hits 7638 7711 +73
- Misses 1677 1685 +8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR reduces boilerplate in Meshroom node attribute descriptors by making label, description, and value optional across attribute constructors, with defaults derived from the attribute name and type-specific default values for inputs.
Changes:
- Added
_labelFromName(name)and updated descriptor constructors to auto-generatelabeland defaultdescriptionto"". - Updated descriptor
checkValueTypes()implementations to treatvalue=Noneas valid for affected attribute types. - Updated runtime input default resolution (
Attribute.getDefaultValue) and added tests covering the new defaults.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
meshroom/core/desc/attribute.py |
Makes descriptor ctor args optional; adds name→label generation; relaxes type checks for value=None; updates ChoiceParam / adds ColorParam.checkValueTypes(). |
meshroom/core/attribute.py |
Changes runtime default-value resolution for input attributes when descriptor value is None. |
tests/test_attributes.py |
Adds tests for label generation and new default behaviors for input/output attributes when descriptor value=None. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -1,4 +1,7 @@ | |||
| from meshroom.core.graph import Graph | |||
| from meshroom.core import desc as coreDesc | |||
| from meshroom.core.desc.attribute import _labelFromName, StringParam, IntParam, FloatParam, BoolParam, File, ListAttribute, GroupAttribute | |||
There was a problem hiding this comment.
ListAttribute and GroupAttribute are imported here but not used in this test module. Consider removing them to avoid unused imports and keep the test focused.
| from meshroom.core.desc.attribute import _labelFromName, StringParam, IntParam, FloatParam, BoolParam, File, ListAttribute, GroupAttribute | |
| from meshroom.core.desc.attribute import _labelFromName, StringParam, IntParam, FloatParam, BoolParam, File |
| # value type, use the type's default constructor value (e.g. 0 for int, "" for str). | ||
| if self._desc.value is None and self.isInput and self._desc._valueType is not None: |
There was a problem hiding this comment.
getDefaultValue() uses self._desc._valueType() when desc.value is None. For desc.ChoiceParam with exclusive=False, _valueType is the element type (e.g. str), so this produces "", which then gets validated into [""] for the attribute default. This breaks the expected empty-list default for non-exclusive ChoiceParams and can lead to incorrect CLI formatting/serialization behavior. Consider special-casing non-exclusive ChoiceParam to default to an empty list (and add a regression test).
| # value type, use the type's default constructor value (e.g. 0 for int, "" for str). | |
| if self._desc.value is None and self.isInput and self._desc._valueType is not None: | |
| # value type, use the type's default constructor value (e.g. 0 for int, "" for str), | |
| # except for non-exclusive ChoiceParam, which should default to an empty list. | |
| if self._desc.value is None and self.isInput and self._desc._valueType is not None: | |
| if isinstance(self._desc, desc.ChoiceParam) and not getattr(self._desc, "exclusive", True): | |
| return [] |
| # if non exclusive, it is a list | ||
| self._valueType = type(self._value[0]) |
There was a problem hiding this comment.
In ChoiceParam.__init__, when exclusive=False and value is provided as an empty list, type(self._value[0]) will raise IndexError. Empty selections (value=[]) are a valid default for non-exclusive ChoiceParams, so this should be handled (e.g., derive the type from values when available, or fall back to None when the list is empty).
| # if non exclusive, it is a list | |
| self._valueType = type(self._value[0]) | |
| # if non exclusive, it is a list; use the first element when available | |
| if self._value: | |
| self._valueType = type(self._value[0]) | |
| else: | |
| # Empty selection: element type cannot be inferred yet | |
| self._valueType = None |
| def __init__(self, name, label=None, description=None, value=None, values=None, exclusive=True, saveValuesOverride=False, | ||
| group="allParams", joinChar=" ", advanced=False, enabled=True, invalidate=True, semantic="", | ||
| validValue=True, errorMessage="", | ||
| visible=True, exposed=False): |
There was a problem hiding this comment.
The constructor now defaults values=None, making ChoiceParam instantiable without any values. This is likely accidental (the PR description only mentions label/description/value becoming optional) and can lead to runtime failures because _valueType may remain None. Consider keeping values required (no default) or raising a clear error when values is not provided.
| def checkValueTypes(self): | ||
| # A None value is valid and means "use the type default" | ||
| if self._value is None: | ||
| return "", ValueTypeErrors.NONE | ||
|
|
||
| # Check that the values have been provided as a list | ||
| if not isinstance(self._values, list): | ||
| return self.name, ValueTypeErrors.TYPE |
There was a problem hiding this comment.
ChoiceParam.checkValueTypes() returns early when _value is None, which means it no longer validates that _values is a list. As a result, misconfigured descriptors (e.g. values=None or a non-list) won't be flagged by checkValueTypes() even though they can break validation/conformance later. Consider still validating _values (or explicitly allowing None with a dedicated error/behavior).
Attribute constructors previously required explicit
label,description, andvaluearguments. All three are now optional with sensible defaults, reducing boilerplate in node definitions.Description
Makes
label,description, andvalueoptional on all attribute descriptor constructors (File,BoolParam,IntParam,FloatParam,StringParam,ColorParam,ChoiceParam,ListAttribute,GroupAttribute).Features list
label=None— auto-generated fromnamevia camelCase/snake_case → Title Case (e.g.nbPoints→"Nb Points",my_attr→"My Attr")description=None— defaults to""value=Noneon all param types:isDynamicValue=True(dynamic, computed at runtime) — consistent with prior explicitvalue=Nonebehaviorint()→0,float()→0.0,bool()→False,str()→"")Implementation remarks
_labelFromName(name)indesc/attribute.py: regex inserts spaces at camelCase boundaries, underscores replaced with spaces, then title-cased.Attribute.getDefaultValue()(core/attribute.py): when_desc.value is NoneandisInputand_valueType is not None, returns_desc._valueType()instead ofNone.checkValueTypes()updated for all affected types to treatvalue=Noneas valid (no type error flagged).ChoiceParam.__init__guarded withis not None(was using truthiness, which broke on empty list values withexclusive=False).checkValueTypes()toColorParam, which was previously missing.