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

DEPR, DOC: Deprecate buffer_lines in read_csv #13360

Closed

Conversation

gfyoung
Copy link
Member

@gfyoung gfyoung commented Jun 4, 2016

buffer_lines is not respected, as it is determined internally via a heuristic involving table_width (see here for how it is computed).

@gfyoung gfyoung force-pushed the buffer-lines-depr-doc branch from 40010c2 to 87eff55 Compare June 4, 2016 02:34
@codecov-io
Copy link

codecov-io commented Jun 4, 2016

Current coverage is 84.23%

Merging #13360 into master will increase coverage by <.01%

@@             master     #13360   diff @@
==========================================
  Files           138        138          
  Lines         50724      50725     +1   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits          42726      42727     +1   
  Misses         7998       7998          
  Partials          0          0          

Powered by Codecov. Last updated by 103f7d3...87eff55

@@ -227,14 +227,19 @@
Note that the entire file is read into a single DataFrame regardless,
use the `chunksize` or `iterator` parameter to return the data in chunks.
(Only valid with C parser)
buffer_lines : int, default None
Copy link
Contributor

Choose a reason for hiding this comment

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

is this even used at all now?

Copy link
Contributor

Choose a reason for hiding this comment

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

it seems its not respected at all now. so we should just remove this argument (or raise if its not None)

Copy link
Member Author

Choose a reason for hiding this comment

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

Based on my PR description, nowhere. How significant of an API change would either option be?

Copy link
Contributor

Choose a reason for hiding this comment

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

well it doesn't do anything now. I guess deprecation is fine. Why don't you re-word to say it currently has no-effect.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fair enough. Done.

@jreback jreback added API Design IO CSV read_csv, to_csv labels Jun 4, 2016
The 'buffer_lines' parameter is not even respected
in the implementation, as it is determined internally
to the C parser.

[ci skip]
@gfyoung gfyoung force-pushed the buffer-lines-depr-doc branch from 87eff55 to a72ecbe Compare June 4, 2016 20:58
@gfyoung
Copy link
Member Author

gfyoung commented Jun 4, 2016

@jreback : Made the requested doc change, and Travis is giving the green light. Ready to merge if there is nothing else.

@jreback jreback added this to the 0.18.2 milestone Jun 5, 2016
@jreback jreback closed this in 863cbc5 Jun 5, 2016
@jreback
Copy link
Contributor

jreback commented Jun 5, 2016

ty

value is not respected by the parser

If ``low_memory`` is ``True``, specify the number of rows to be read for
each chunk. (Only valid with C parser)
Copy link
Member

Choose a reason for hiding this comment

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

Can't we leave out the actual explanation? As this is not only deprecated, but also does not work (IIUC), so it does not serve much purpose IMO (apart from explaining what feature exactly has never worked, and will never work ..)

BTW, @gfyoung, for the rest strong +1 on your cleaning up of the keywords!

Copy link
Member Author

Choose a reason for hiding this comment

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

@jorisvandenbossche : Will do. Thanks, for the +1 - I use this function a great deal in my own code, so I'm certainly more than happy to improve it given how much it has done for me, even in such a "broken" state. 😄

gfyoung added a commit to forking-repos/pandas that referenced this pull request Dec 19, 2017
jreback pushed a commit that referenced this pull request Dec 19, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Design IO CSV read_csv, to_csv
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants