Skip to content
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

Added nextRecord to ExcelRecordReader to prevent a bug with hasNext() calls #5758

Merged
merged 2 commits into from Jul 3, 2018

Conversation

@yquemener
Copy link
Contributor

commented Jun 30, 2018

calling nextRecord on an ExcelRecordReader would call the super() function, which results in returning raw binaries from the file.

@AlexDBlack
Copy link
Contributor

left a comment

This LGTM, but I haven't run tests or anything.
Is this good to merge?

@yquemener

This comment has been minimized.

Copy link
Contributor Author

commented Jul 2, 2018

The tests failed even before that modification because a file named datavec-excel/testsheet.xlsx is nowhere to be found in the repository. That may be another thing to repair.

@AlexDBlack

This comment has been minimized.

Copy link
Contributor

commented Jul 2, 2018

@yquemener All datavec tests are passing (at least last time I checked - last weekd), and have been for a while...
The test resources you are referring to is here: https://github.com/deeplearning4j/dl4j-test-resources/tree/master/src/main/resources/datavec-excel
Just make sure you have dl4j-test-resources installed, and have the appropriate profile enabled (test-nd4j-native)

@yquemener

This comment has been minimized.

Copy link
Contributor Author

commented Jul 2, 2018

Thanks, the tests now run and show an error! Correcting it now.

@AlexDBlack
Copy link
Contributor

left a comment

LGTM - assuming tests pass now, of course :)

@AlexDBlack

This comment has been minimized.

Copy link
Contributor

commented Jul 3, 2018

@yquemener can this be merged now?

@agibsonccc agibsonccc merged commit 48d7590 into eclipse:master Jul 3, 2018

1 of 2 checks passed

codeclimate Code Climate encountered an error attempting to analyze this pull request.
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details
@yquemener

This comment has been minimized.

Copy link
Contributor Author

commented Jul 4, 2018

Yes, thanks!

@yquemener

This comment has been minimized.

Copy link
Contributor Author

commented Jul 4, 2018

This one would be useful to Discover too: #5782

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.