-
Notifications
You must be signed in to change notification settings - Fork 94
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
Xtrig arg validate #5955
Xtrig arg validate #5955
Conversation
…examples for each of the built in xtriggers. - Xrandom validate function - Init test xrandom validate function - Add unit tests for validation of built in xtriggers
@markgrahamdawson requesting a second review because this is quite collab between Hilary and I at this point. |
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 realised it's trivially simple to automatically ensure xtrigger function args match the function signature at validation time
PR open here: wxtim#59
* Remove redundant try block. * Fix changelog * Automatically validate xtrigger function signature --------- Co-authored-by: Hilary James Oliver <[email protected]>
Co-authored-by: Ronnie Dutta <61982285 [email protected]>
Fixed locally. Reopened so CI runs. Hopefully works on CI too. |
This comment was marked as resolved.
This comment was marked as resolved.
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.
While looking at the test failures, I realised the implementation of xrandom
's validate
function was flawed with respect whether you used args vs kwargs.
Then I realised I could simplify the way validation is carried out, PR open at wxtim#60
* Improve xtrigger validation * wall_clock: use placeholder function for signature validation & autodocs * Fix docstring for autodoc [skip ci]
Kicking tests |
Ah right, @wxtim when you squash merge a PR make sure you remove |
@wxtim Any objection to merging this? I think enough due process has occurred at this point, seeing as I have reviewed yours/Hilary's code and you've reviewed mine? |
That seems reasonable - All the code has been reviewed by two pairs of eyes in effect even if the review was not linear. |
Close #5448
Replaces #5452, to which I have added tests.
From @hjoliver
From @MetRonnie (added to this PR)
Check List
CONTRIBUTING.md
and added my name as a Code Contributor.setup.cfg
(andconda-environment.yml
if present).CHANGES.md
entry included if this is a change that can affect users?.?.x
branch.