Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Change NewSplitDriver paramater and initialization #1798
Change NewSplitDriver paramater and initialization #1798
Changes from 10 commits
dd93ab7
54f68d1
d47871b
81fcd4e
e4bdb6d
6743e4d
f1c17af
e0b1b2b
06fdd2d
dd623a8
cfa6a91
9493d37
8b9bc71
2694914
419e4fc
5e6841b
06acaa7
6ff5b28
4f5547d
845fc07
c9d4971
2a081bb
2f0aa62
c9b5d1e
d6a784d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 know this makes no actual difference in the code, but could we change these nils to actual snapshots like the test above?
This is just to make sure future iterations of this can't shortcut nil or 0 len inputs and still pass but fail with real data.
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.
It would make sense to just abstract this part of the repeated code into a separate function that can be called and will ensure this uniform call.
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.
That makes a lot of sense. I will work on refining the test cases, was trying to get the core change out for review. Do you want me to change this to a draft PR @MrAlias ?
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.
@bryan-aguilar if you think the changes needed will be greater than just the two tests here doing in another PR sounds reasonable, but if not including the changes here seems valid.