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

- dropped SKIP_EMPTY and READ_AHEAD due to strange behavior when using c... #25

Merged
merged 6 commits into from
Sep 5, 2014

Conversation

jorgecolonconsulting
Copy link
Collaborator

  • dropped SKIP_EMPTY and READ_AHEAD due to strange behavior when using current(), seek(), and rewind(). Skipped empty rows instead with code.
  • getLineNumber now uses SplFileObject::key() for accurate numbers
  • getLastLineNumber(), for possibly splitting up a large CSV file and processing in a queue
  • getCurrentRow() to accurately get a array from the current line
  • advanceTo() to move to a file line
  • setHeaderLine() to set a header line that's not the first row
  • removed incrementLine() since the core is using SplFileObject for seeking, the method would make line numbers inaccurate
  • rowIsEmpty() to check if it's a completely empty line or empty with delimiters
  • Unit tests and fixtures to test all new and refactored behavior

This is a pretty major shift in the core code so if you see any issues let me know. I got about 87% official coverage from Xdebug for the Reader class, but I'm sure it's more. For some reason the constructor is getting called, but the code coverage doesn't show that those lines are getting called.

…g current(), seek(), and rewind(). Skipped empty rows instead with code.

- getLineNumber now uses SplFileObject::key() for accurate numbers
- getLastLineNumber(), for possibly splitting up a large CSV file and processing in a queue
- getCurrentRow() to accurately get a array from the current line
- advanceTo() to move to a file line
- setHeaderLine() to set a header line that's not the first row
- removed incrementLine() since the core is using SplFileObject for seeking, the method would make line numbers inaccurate
- rowIsEmpty() to check if it's a completely empty line or empty with delimiters
- Unit tests and fixtures to test all new and refactored behavior
/**
* @dataProvider getReadersNoHeadersFirstRow
*/
public function testGetLastLineNumberNoHeadersFirstRow( Reader $reader )
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would be good to remove these spaces around, you can also see https://github.com/fabpot/php-cs-fixer

@jorgecolonconsulting
Copy link
Collaborator Author

@cordoval ok. Took care of those.

*/
public function getLastLineNumber()
{
if( $this->lastLine !== false ) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code formatting here is off.

@jwage
Copy link
Owner

jwage commented Sep 3, 2014

Just a few coding standard/formatting issues.

@jorgecolonconsulting
Copy link
Collaborator Author

@jwage alright. How does this look now? I ran the php-cs-fixer that was suggested up top.

@jorgecolonconsulting
Copy link
Collaborator Author

@jwage just giving this a little bump

/**
* @var bool
*/
private $headerLine = false;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a space between this property and the doc block below.

@jwage
Copy link
Owner

jwage commented Sep 5, 2014

👍

@jwage
Copy link
Owner

jwage commented Sep 5, 2014

I will merge if you can just fix those two spacing issues.

@jorgecolonconsulting
Copy link
Collaborator Author

@jwage Badda bing. Badda boom.

jwage added a commit that referenced this pull request Sep 5, 2014
- dropped SKIP_EMPTY and READ_AHEAD due to strange behavior when using c...
@jwage jwage merged commit ac26d47 into jwage:master Sep 5, 2014
@jwage
Copy link
Owner

jwage commented Sep 5, 2014

Thanks for your contribution! :)

@jorgecolonconsulting
Copy link
Collaborator Author

You're welcome! What coding style are you using?

On Sep 5, 2014, at 11:48 AM, "Jonathan H. Wage" notifications@github.com wrote:

Thanks for your contribution! :)


Reply to this email directly or view it on GitHub.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants