-
Notifications
You must be signed in to change notification settings - Fork 27
Reccaster exclusion pattern for record filtering #112
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
Conversation
e7b2766
to
f4c8f6d
Compare
squashed all commits |
c4d4266
to
5f43a89
Compare
@madelinespark and I looked through and cleaned up things a bit in
we were wondering if |
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.
Looks good to me and tested with base R3.15.8 and R7.0.9. ELLLIST is very handy!
Hi @simon-ess , sorry for another ping but there have been enough changes since your last approval that we would appreciate one final look through |
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!
Note that you're failing a sonarqube check for duplicated code on the test function. It might be good to refactor that code, but you also could consider disabling the check on test code? |
I'd be in favor of removing that check on the test file. @mdavidsaver comments on #117 (comment) would apply to this PR as well. The PRs go in tandem and I had Madeline create the addReccasterEnvVars refactor PR on Friday since that was the last day of her internship. It branches off this PR since there is shared code like
edit - actually maybe it makes more sense to get this PR fully completed and then worry about pr #117 |
Yeah, I saw that that other PR was based off of this one and sort of assumed that it would be rebased after this one is merge. |
|
The changes Michael mentioned about array allocation and looping through the linked lists are implemented. Also updated the variable naming as suggested. This is ready to merge from our end, can you take one last look @simon-ess ? |
No description provided.