-
Notifications
You must be signed in to change notification settings - Fork 1
WIP: Spout implementation #1
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
base: master
Are you sure you want to change the base?
Conversation
Baachi
left a comment
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.
Hey @aaa2000
thanks for your contribution! Really appreciated.
Looking forward to merging this PR.
src/SpoutReader.php
Outdated
| */ | ||
| public function __construct(\SplFileObject $file, $headerRowNumber = null, $activeSheet = null, $shouldPreserveEmptyRows = true) | ||
| { | ||
| $reader = $this->createReaderForFile($file, $shouldPreserveEmptyRows); |
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 would rather inject the ReaderInterface directly.
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.
Fixed, the user must call the ReaderInterface::open method before use the SpoutReader
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.
With this fix, the SpoutReader is compatible with CSV, XLSX and ODS.
But the method count of the ODS format throw an exception because the RowIterator can't be rewind more than once. See https://github.com/box/spout/blob/3681a3421a868ab9a65da156c554f756541f452b/src/Spout/Reader/ODS/RowIterator.php#L101
Maybe, I will not implement the CountableReader interface
| */ | ||
| public function setHeaderRowNumber($rowNumber) | ||
| { | ||
| $rowNumber = (int) $rowNumber; |
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 would rather use a static type hint for this method.
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.
Need to require PHP7
src/SpoutReader.php
Outdated
| * | ||
| * @return array | ||
| */ | ||
| public function getRow($number) |
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.
What is the use case for this method?
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.
Removed
I have added it because the ExcelReader implement it https://github.com/portphp/excel/blob/458d5cf105206f6391e4350ee095110d18eda090/src/ExcelReader.php#L201
composer.json
Outdated
| }, | ||
| "require": { | ||
| "php": ">=5.4.0" | ||
| "php": ">=5.5.0", |
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.
We should require at least php 5.6, because portphp/portphp requires at least php 5.6.
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.
Fixed
I tried to implement the
spoutreader and writer.Spoutread and write spreadsheet files in CSV, XLSX and ODS. But, the reader is only compatible with XLSX.I based myself on the code of
portphp/excel, the fixtures are XLS format butspoutdoesn't manage this format. I converted the XLS file in XLSX format withlibreofficeThe writer can't edit an existing spreadsheet, the data will be overwritten.
http://opensource.box.com/spout/guides/add-data-to-existing-spreadsheet/
http://opensource.box.com/spout/guides/edit-existing-spreadsheet/
Close portphp/portphp#61