-
Notifications
You must be signed in to change notification settings - Fork 15
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
Remove eval called by rrule_test #32
Conversation
Is there anything else this PR needs? |
sorry its been weekend+public holidays here since it was openned. |
Co-authored-by: Matt.jl <matt.brzezinski@invenia.ca>
I am going to leave this to @mattBrzezinski to review and follow up til merged. |
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.
LGTM, just two minor whitespacing comments, and then making f2
definition the first thing inside of _make_fdm_call
for increased readability!
Co-authored-by: Matt.jl <matt.brzezinski@invenia.ca>
Thanks! Implemented the requested changes. |
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.
LGTM. Thanks! A other couple tiny comments which you're free to ignore.
Before we merge, please can you bumpy the version number?
I know it's an internal-only change, but good to release this change by itself and not with other changes in case we do hit any unexpected issues
Co-authored-by: Nick Robinson <npr251@gmail.com>
Done! |
This fixes #30 by removing the "terrifying"
eval
in_make_fdm_call
.