Skip to content

csv readers take iterables as well as iterators #2653

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

Closed
wants to merge 1 commit into from

Conversation

graingert
Copy link
Contributor

No description provided.

@mention-bot
Copy link

@graingert, thanks for your PR! By analyzing the history of the files in this pull request, we identified @bitdancer, @berkerpeksag and @birkenfeld to be potential reviewers.

@the-knights-who-say-ni
Copy link

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA (this might be simply due to a missing "GitHub Name" entry in your b.p.o account settings). This is necessary for legal reasons before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

Thanks again to your contribution and we look forward to looking at it!

@graingert
Copy link
Contributor Author

@Mariatta Mariatta added the docs Documentation in the Doc dir label Jul 11, 2017
string each time its :meth:`!__next__` method is called --- :term:`file objects
*csvfile* can be any object which supports the :term:`iterable` protocol and returns an
iterator that returns a string each time its :term`iterator`'s :meth:`!__next__` method
is called when its :meth:`!__iter__` method is called --- :term:`file objects
Copy link
Member

Choose a reason for hiding this comment

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

I would suggest instead changing the original with the following minimal change:
...can be any iterable object whose iterator returns a string each time its...

@bitdancer
Copy link
Member

I don't think this is completely trivial, so you should open an issue for it.

@@ -416,7 +417,7 @@ Reader objects have the following public attributes:

.. attribute:: csvreader.line_num

The number of lines read from the source iterator. This is not the same as the
The number of lines read from the source iterable. This is not the same as the
Copy link
Member

Choose a reason for hiding this comment

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

Why change this? If there is only one iterator involved, the original would be more precise.

Copy link
Member

Choose a reason for hiding this comment

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

That's a good point, iterator is more correct here.

@brettcannon
Copy link
Member

To try and help move older pull requests forward, we are going through and backfilling 'awaiting' labels on pull requests that are lacking the label. Based on the current reviews, the best we can tell in an automated fashion is that a core developer requested changes to be made to this pull request.

If/when the requested changes have been made, please leave a comment that says, I have made the requested changes; please review again. That will trigger a bot to flag this pull request as ready for a follow-up review.

@graingert
Copy link
Contributor Author

Sadly I am unable to honestly say that

I have made the requested changes; please review again.

@bedevere-bot
Copy link

Thanks for making the requested changes!

@bitdancer: please review the changes made to this pull request.

@graingert
Copy link
Contributor Author

However it was requested that I make an 'issue' first

@brettcannon
Copy link
Member

Closing as the CLA has not been signed within the last month.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting changes docs Documentation in the Doc dir
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants