-
Notifications
You must be signed in to change notification settings - Fork 320
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.
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
add vals property and setter to delegate_parameter #6817
base: main
Are you sure you want to change the base?
add vals property and setter to delegate_parameter #6817
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6817 +/- ##
==========================================
+ Coverage 69.37% 69.39% +0.01%
==========================================
Files 340 340
Lines 31341 31355 +14
==========================================
+ Hits 21744 21758 +14
Misses 9597 9597 ☔ View full report in Codecov by Sentry. |
@@ -314,3 +335,5 @@ def validate(self, value: ParamDataType) -> None: | |||
super().validate(value) | |||
if self.source is not None: | |||
self.source.validate(self._from_value_to_raw_value(value)) | |||
if self.vals is not None: | |||
self.vals.validate(value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this necessary. Will the call to super.validate above not do the same?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it will only validate with the validator on the source, so it will not enforce the validator on the delegate parameter without this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The additional cases I added in the tests show this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see what is going on. the call to validator in the base class will use all validators stored as part of _vals. Since this pr prevents you from adding any validator to that list it will always be empty so that line does nothing on this pr. However prior to this pr adding a validator to the DelegateParameter would cause it to be added to the parameters _vals list and used. This makes me suspect that the tests as is would also pass on main. #6832 seems to confirm this too so I am not completely sure if this pr is required
elif self.source is not None: | ||
return self.source.vals | ||
else: | ||
return None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ParameterBase supports multiple validators via the add_validator etc logic. We need to consider if we want to support that here too.
Add
vals
property and@vals.setter
to qcodes.parameters.delegate_parameter.DelegateParameter to propagate the Validator from source to DelegateParameter.