-
Notifications
You must be signed in to change notification settings - Fork 11.9k
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
Bugfix/4087 #4590
Bugfix/4087 #4590
Conversation
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 didn't see anything majorly wrong with the code. Looks like some tests were affected however, so that needs to be fixed before this can be merged
@etimberg it looks like the tests have been fixed for this PR now |
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.
Thanks @derekellis (would need to resolve conflicts before merge)
@derekellis can you rebase this PR so that we can merge it? |
@derekellis are you still able to rebase this? |
@derekellis it looks like this PR is ready to be merged if you can rebase it |
@derekellis thanks for all the effort on this. we'd like to be able to merge, but may end up closing this PR if you aren't able to rebase it |
Closing this for now. Please feel free to reopen it once you've rebased it |
Since @derekellis has gone dark is this not something that ya'll or someone else could add into the codebase to fix the problem? I'm happy to submit a PR with a rebased version of the code changes above if that would work. |
My proposed fix for issue #4087