Skip to content

Fixing 'names' doc for 'read_csv' #30109

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

Merged
merged 1 commit into from
Dec 8, 2019

Conversation

bessarabov
Copy link
Contributor

  • closes #xxxx
  • tests added / passed
  • passes black pandas
  • passes git diff upstream/master -u -- "*.py" | flake8 --diff
  • whatsnew entry

I think there is a bug in the docs. This Jupyter notebook shows that the code does not work as expected from the docs https://gist.github.com/bessarabov/5e42bbb359fb86aaa87aed6c12a95bc9

So I've fixed the doc to match behaviour.

@@ -120,7 +120,7 @@
data rather than the first line of the file.
names : array-like, optional
List of column names to use. If file contains no header row, then you
should explicitly pass ``header=None``. Duplicates in this list are not
Copy link
Member

@alimcmaster1 alimcmaster1 Dec 6, 2019

Choose a reason for hiding this comment

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

‘If the file contains a header row, then you should explicitly pass ‘’header=0’’ to override the column names.’ - I think that would make more sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you. I've rechecked who the system is working (I've added the comment with the details) and I have updated the text as you have suggested.

@bessarabov bessarabov force-pushed the fix_read_csv_names_doc branch from 3bf26f1 to 6c5b1e0 Compare December 7, 2019 08:08
@bessarabov
Copy link
Contributor Author

I made one more python notebook to check how the code works when there is no header in csv:

The current doc text is:

names : array-like, optional
List of column names to use. If file contains no header row, then you should explicitly pass
header=None. Duplicates in this list are not allowed.

As you can see from pandas read_csv no-header.ipynb

  • [pandas read_csv with-header.ipynb] there is no need to explicitly pass header=None the code works without header=None as it does with header=None

So I think there should be no such text in the doc.

But when there is a header in csv file you must specify header=0 to make the code work as expected — pandas read_csv with-header.ipynb

List of column names to use. If file contains no header row, then you
should explicitly pass ``header=None``. Duplicates in this list are not
allowed.
List of column names to use. If the file contain a header row,
Copy link
Member

Choose a reason for hiding this comment

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

Sorry minor typo from me should be ‘file contains’

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops. Fixed, thank you.

@bessarabov bessarabov force-pushed the fix_read_csv_names_doc branch from 6c5b1e0 to ba0cff3 Compare December 7, 2019 12:45
@alimcmaster1 alimcmaster1 self-requested a review December 7, 2019 18:28
Copy link
Member

@alimcmaster1 alimcmaster1 left a comment

Choose a reason for hiding this comment

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

LGTM

@jreback jreback added this to the 1.0 milestone Dec 8, 2019
@jreback jreback merged commit 95b9c2f into pandas-dev:master Dec 8, 2019
@jreback
Copy link
Contributor

jreback commented Dec 8, 2019

thanks @bessarabov

@bessarabov bessarabov deleted the fix_read_csv_names_doc branch December 8, 2019 17:30
proost pushed a commit to proost/pandas that referenced this pull request Dec 19, 2019
proost pushed a commit to proost/pandas that referenced this pull request Dec 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants