-
Notifications
You must be signed in to change notification settings - Fork 220
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
variable naming / destructuring #2465
variable naming / destructuring #2465
Conversation
Of course, feel free to push back on any of it like normal review comments:) |
aacb4ed
to
e18bbac
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## mhauru/recursive-gibbs #2465 +/- ##
=======================================================
Coverage 85.01% 85.01%
=======================================================
Files 21 21
Lines 1582 1582
=======================================================
Hits 1345 1345
Misses 237 237 ☔ View full report in Codecov by Sentry. |
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.
Happy with the other changes, but see comment above for varname_tuples
.
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.
Good changes, thanks!
* Use recursion in gibbs_inner_step * Remove unnecessary initialisation in Gibbs We were doing work that was already done by the caller of initialstep. * variable naming / destructuring (#2465) * Variable naming, destructuring * Tuple -> Vec * Reviewdog code style suggestions Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> --------- Co-authored-by: Penelope Yong <[email protected]> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
I thought I'd put some code suggestions in a PR rather than review comments, because to suggest all these changes would have been fiddly and error-prone.
Basically, I'm proposing these changes:
_local
variable names are a bit repetitive and have lost their meaning since the loop was extracted into a function, so I got rid of most of them.varname_tuples
, as that better reflects the fact that there are two nested levels of containers (as opposed tovarnames
which suggests only one level)h, t... = container
as opposed to explicit indexing[2:end]
, as I think it's cleaner and better reflects what we're trying to do (pluck off the first element and keep the rest for later)