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

Deprecation of League\Csv\Reader::fetch #208

Closed
JC5 opened this issue Jan 30, 2017 · 18 comments
Closed

Deprecation of League\Csv\Reader::fetch #208

JC5 opened this issue Jan 30, 2017 · 18 comments

Comments

@JC5
Copy link

JC5 commented Jan 30, 2017

League\Csv\Reader::fetch has been marked as deprecated as per the change log but it is still used as a main example in the "reading" section of the manual and there does not seem to be any indication this method is actually deprecated, nor what the alternative should be.

@nyamsprod
Copy link
Member

nyamsprod commented Jan 31, 2017

@JC5 yes indeed it is marked as deprecated because work has started on the next major release and Reader::fetch will be indeed removed from the public API in the next major release. This is the reason it is marked as deprecated. There's no alternative right now for its usage but marking it like that is a hint to users that this method will be removed. AFAIK this does not in any way prevents anyone from using the method right now.

Hope this answer your question as it is similar to #206

@JC5
Copy link
Author

JC5 commented Jan 31, 2017

It does, apologies. I am used to deprecation after the new method has been presented. 😉

@JC5 JC5 closed this as completed Jan 31, 2017
@nyamsprod nyamsprod reopened this Jan 31, 2017
@joshbrw
Copy link

joshbrw commented Feb 9, 2017

I came here for this exact reason. Shouldn't the method be deprecated once an alternative is available?

@nyamsprod
Copy link
Member

@joshbrw Please refer to PHPdoc documentation

@joshbrw
Copy link

joshbrw commented Feb 13, 2017

@nyamsprod Surely these methods are going to be superceded by some other method? The PHPDoc documentation states If it is superceded by another method it is RECOMMENDED to add a @see tag in the same PHPDoc pointing to the new element., which is what I'd have expected.

@nyamsprod
Copy link
Member

nyamsprod commented Feb 13, 2017

If it is superceded by another method

there's a If at the start of the sentence :)

@joshbrw
Copy link

joshbrw commented Feb 13, 2017

Of course. But surely you're always going to need to fetch data from the CSV, no?

@nyamsprod
Copy link
Member

yes and having a @deprecated in the docblock does not mean that the method won't work anymore.. it is an indication about where the package will go next. Really I don't see the problem with this ? This is informative only.

@joshbrw
Copy link

joshbrw commented Feb 13, 2017

I guess it all depends on your view of what deprecated means. My IDE (PHPStorm) throws up a usage warning if the method is used.

@nyamsprod
Copy link
Member

I guess it all depends on your view of what deprecated means

Nope. The definition of @deprecated is the one PHPdoc put out and this is the one I'm following. I could rephrase that by saying that It depends on how PHPStorm interprets it :) .

@cdekok
Copy link

cdekok commented Feb 20, 2017

@nyamsprod static analyzers don't like it either and any ide.. if there is no alternative it's not nice to mark it as deprecated.

@joshbrw
Copy link

joshbrw commented Feb 20, 2017

@cdekok I don't think we're going to get our way with this one, but I whole-heartedly agree with you.

nyamsprod pushed a commit that referenced this issue Feb 22, 2017
- Remove the @deprecate message from the Reader class #208
- Bug fix internal Reader::getRow when used with a StreamIterator #213
nyamsprod added a commit that referenced this issue Feb 22, 2017
- Remove the @deprecate message from the Reader class #208
- Bug fix internal Reader::getRow when used with a StreamIterator #213
@alexeyshockov
Copy link

Agree with @joshbrw. When I see a deprecated method, I always try to refactor the code. But in this case I'm unable to do that.

By using @deprecated just to notify users, you push them to pay less attention to really deprecated things (because they cannot fix all @deprecated issues from now). And after some time with a bunch of such issues unresolved you just stop paying attention to them.

If it is superceded by another method

Of course, you are right that providing an alternative is not required. But Reader is the only class to read content from a CSV file (in the library), and, deprecating of its main methods without providing an alternative, you deprecate the whole task of reading CSV.

So I'm puzzled now, because I want to read a CSV file, but I don't know how to do it properly (with league/csv).

Just my 5 cents.

@JC5
Copy link
Author

JC5 commented Feb 22, 2017

I appreciate the change, @nyamsprod !

@nyamsprod
Copy link
Member

nyamsprod commented Feb 22, 2017

At everyone, I had a discussion off record with other leaguers and I decided to make a patch release which will remove most of the @deprecated. Once the others bug fixes are reviewed the patch will be released later this week or next week.

@joshbrw
Copy link

joshbrw commented Feb 22, 2017

Thanks @nyamsprod - appreciate it!

nyamsprod added a commit that referenced this issue Feb 23, 2017
* Bug fix #213 and #208

- Remove the @deprecate message from the Reader class #208
- Bug fix internal Reader::getRow when used with a StreamIterator #213
- Bug fix StreamIterator::fputcsv
@nyamsprod
Copy link
Member

version 8.2.1 is out with the fix

@alexeyshockov
Copy link

Thanks, @nyamsprod!

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

No branches or pull requests

5 participants