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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Override the createFromPath static method for a simple open mode on Reader #266

Merged
merged 2 commits into from Oct 13, 2017

Conversation

Projects
None yet
3 participants
@btmash
Contributor

btmash commented Oct 12, 2017

Introduction

I actually got bit by the readonly issue that is described in #258 And figured I'd give a shot at how to fix the issue.

Proposal

I introduce a new static method in reader where the default open mode is 'r'. I also added a test to ensure this works by adding a readonly csv file.

Describe the new/upated/fixed feature

I override the static function createFromPath provided by AbstractCsv to set the default open mode.

Backward Incompatible Changes

There shouldn't be a BC issue.

Targeted release version

Could be 9.0, 10.0, up to you. This is my first time writing a PR for a PHP library like this so its very much a learning experience for me 馃槃

PR Impact

I don't think there should be a PR impact.

Open issues

#258

@@ -62,6 +62,14 @@ class Reader extends AbstractCsv implements Countable, IteratorAggregate, JsonSe
protected $stream_filter_mode = STREAM_FILTER_READ;
/**
* @inheritdoc
*/
public static function createFromPath(string $path, string $open_mode = 'r', $context = null): AbstractCsv

This comment has been minimized.

@nyamsprod

nyamsprod Oct 12, 2017

Member

This introduces a tiny BC break you can't just correct it like

@nyamsprod

nyamsprod Oct 12, 2017

Member

This introduces a tiny BC break you can't just correct it like

This comment has been minimized.

@btmash

btmash Oct 12, 2017

Contributor

Since you'd mentioned this would potentially go in a 10.0 release (which is likely to be atleast a year away), I approached it with that in mind. Is there another way I should approach this PR?

@btmash

btmash Oct 12, 2017

Contributor

Since you'd mentioned this would potentially go in a 10.0 release (which is likely to be atleast a year away), I approached it with that in mind. Is there another way I should approach this PR?

This comment has been minimized.

@Jalle19

Jalle19 Mar 3, 2018

Contributor

The return type-hint here is wrong. You're returning new static so the return type should be Reader. While upgrading from 9.0.1 to 9.1.2 I started getting errors like these from Phan:

app/Legacy/Console/Commands/AbstractCsvImportCommand.php:77 PhanUndeclaredMethod Call to undeclared method \League\Csv\AbstractCsv::setHeaderOffset
@Jalle19

Jalle19 Mar 3, 2018

Contributor

The return type-hint here is wrong. You're returning new static so the return type should be Reader. While upgrading from 9.0.1 to 9.1.2 I started getting errors like these from Phan:

app/Legacy/Console/Commands/AbstractCsvImportCommand.php:77 PhanUndeclaredMethod Call to undeclared method \League\Csv\AbstractCsv::setHeaderOffset

@nyamsprod nyamsprod merged commit acd14b8 into thephpleague:master Oct 13, 2017

2 checks passed

Scrutinizer 1 updated code elements
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment