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

DOC: Reword doc for filepath_or_buffer in read_csv #22058

Merged

Conversation

gfyoung
Copy link
Member

@gfyoung gfyoung commented Jul 25, 2018

Shortens dtype description following colon and moves it to a more expanded parameter description.

Closes #22057.

cc @WillAyd

@gfyoung gfyoung added Docs IO CSV read_csv, to_csv labels Jul 25, 2018
@gfyoung gfyoung added this to the 0.24.0 milestone Jul 25, 2018
@pep8speaks
Copy link

pep8speaks commented Jul 25, 2018

Hello @gfyoung! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on July 26, 2018 at 08:37 Hours UTC

@gfyoung gfyoung requested a review from jreback July 25, 2018 23:22
@gfyoung gfyoung force-pushed the read-csv-file-path-docs branch from 45a1517 to 1599a73 Compare July 25, 2018 23:22
If you want to pass in a path object, pandas accepts the following:

* pathlib.Path
* py._path.local.LocalPath
Copy link
Member

Choose a reason for hiding this comment

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

Is this supposed to be private? Not familiar with this but if that's the case don't feel like its worth mention here

Copy link
Member Author

Choose a reason for hiding this comment

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

The alias is py.path.local, so it is in fact public. However, the alias resolves to that "private class", so no need to provide indirection for something like this.

Copy link
Member

Choose a reason for hiding this comment

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

Then I would use the public alias? or do they use it like that as well in their documentation?

Since it are only two, I would maybe put it in a normal paragraph instead of the bullet points (now it seems to get more weight than the other paragraphs)

@codecov
Copy link

codecov bot commented Jul 26, 2018

Codecov Report

Merging #22058 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #22058   +/-   ##
=======================================
  Coverage   92.02%   92.02%           
=======================================
  Files         170      170           
  Lines       50710    50710           
=======================================
  Hits        46664    46664           
  Misses       4046     4046
Flag Coverage Δ
#multiple 90.42% <ø> (ø) ⬆️
#single 42.31% <ø> (ø) ⬆️
Impacted Files Coverage Δ
pandas/io/parsers.py 95.46% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2c7c797...92cc578. Read the comment docs.

Copy link
Member

@jorisvandenbossche jorisvandenbossche left a comment

Choose a reason for hiding this comment

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

A big improvement anyhow!

The string could be a URL. Valid URL schemes include http, ftp, s3, and
file. For file URLs, a host is expected. For instance, a local file could
be file://localhost/path/to/table.csv
filepath_or_buffer : str, path object, or file object
Copy link
Member

Choose a reason for hiding this comment

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

maybe "file-like" instead of "file" ? as eg StringIO is not really a actual file

Copy link
Member Author

Choose a reason for hiding this comment

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

Then I would use the public alias? or do they use it like that as well in their documentation?

https://py.readthedocs.io/en/latest/path.html

py._path.local.LocalPath is the official class in the docs AFAICT. IMO, the naming makes it clearer that it's a class (vs. the all-lowercase py.path.local)

Copy link
Member

Choose a reason for hiding this comment

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

OK, if they use it like that, that's fine

If you want to pass in a path object, pandas accepts the following:

* pathlib.Path
* py._path.local.LocalPath
Copy link
Member

Choose a reason for hiding this comment

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

Then I would use the public alias? or do they use it like that as well in their documentation?

Since it are only two, I would maybe put it in a normal paragraph instead of the bullet points (now it seems to get more weight than the other paragraphs)

Shortens dtype description following colon and
moves it to a more expanded parameter description.

Closes pandas-devgh-22057.
@gfyoung gfyoung force-pushed the read-csv-file-path-docs branch from 1599a73 to 92cc578 Compare July 26, 2018 08:37
@gfyoung
Copy link
Member Author

gfyoung commented Jul 26, 2018

@jreback @jorisvandenbossche : Made the requested changes, and all is green. PTAL.

@jorisvandenbossche jorisvandenbossche merged commit dfd58e8 into pandas-dev:master Jul 27, 2018
@jorisvandenbossche
Copy link
Member

Thanks!

@gfyoung gfyoung deleted the read-csv-file-path-docs branch July 27, 2018 08:40
minggli added a commit to minggli/pandas that referenced this pull request Jul 28, 2018
* master:
  BENCH: asv csv reading benchmarks no longer read StringIO objects off the end (pandas-dev#21807)
  BUG: df.agg, df.transform and df.apply use different methods when axis=1 than when axis=0 (pandas-dev#21224)
  BUG: bug in GroupBy.count where arg minlength passed to np.bincount must be None for np<1.13 (pandas-dev#21957)
  CLN: Vbench to asv conversion script (pandas-dev#22089)
  consistent docstring (pandas-dev#22066)
  TST: skip pytables test with not-updated pytables conda package (pandas-dev#22099)
  CLN: Remove Legacy MultiIndex Index Compatibility (pandas-dev#21740)
  DOC: Reword doc for filepath_or_buffer in read_csv (pandas-dev#22058)
  BUG: rolling with MSVC 2017 build (pandas-dev#21813)
Sup3rGeo pushed a commit to Sup3rGeo/pandas that referenced this pull request Oct 1, 2018
Shortens dtype description following colon and
moves it to a more expanded parameter description.

Closes pandas-devgh-22057.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Docs IO CSV read_csv, to_csv
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants