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

csv module: Add return value to DictWriter.writeheader #71684

Closed
lsowen mannequin opened this issue Jul 12, 2016 · 22 comments
Closed

csv module: Add return value to DictWriter.writeheader #71684

lsowen mannequin opened this issue Jul 12, 2016 · 22 comments
Assignees
Labels
3.8 only security fixes easy type-feature A feature request or enhancement

Comments

@lsowen
Copy link
Mannequin

lsowen mannequin commented Jul 12, 2016

BPO 27497
Nosy @smontanaro, @rhettinger, @vstinner, @bitdancer, @berkerpeksag, @zware, @ashishnitinpatil, @lsowen, @mblahay
PRs
  • bpo-27497: Add return value to csv.DictWriter.writeheader #12306
  • Files
  • issue_27497_add_return_to_DictWriter_writeheader.patch: Patch for issue 27497
  • issue_27497_add_return_to_DictWriter_writeheader_with_documentation_changes.patch: Patch for issue 27497 with suggested documentation changes
  • issue_27497_add_return_to_DictWriter_writeheader_v2.patch: Patch for issue 27497 with suggested documentation changes and unittests
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = 'https://github.com/smontanaro'
    closed_at = <Date 2019-05-10.01:52:50.157>
    created_at = <Date 2016-07-12.15:14:08.016>
    labels = ['easy', 'type-feature', '3.8']
    title = 'csv module: Add return value to DictWriter.writeheader'
    updated_at = <Date 2019-05-10.01:52:50.156>
    user = 'https://github.com/lsowen'

    bugs.python.org fields:

    activity = <Date 2019-05-10.01:52:50.156>
    actor = 'vstinner'
    assignee = 'skip.montanaro'
    closed = True
    closed_date = <Date 2019-05-10.01:52:50.157>
    closer = 'vstinner'
    components = []
    creation = <Date 2016-07-12.15:14:08.016>
    creator = 'lsowen'
    dependencies = []
    files = ['44364', '44365', '44372']
    hgrepos = []
    issue_num = 27497
    keywords = ['patch', 'easy']
    message_count = 22.0
    messages = ['270248', '270250', '270268', '270276', '270340', '270341', '270346', '270363', '270388', '270389', '270407', '274370', '274371', '274398', '274400', '274402', '341556', '341559', '341566', '341793', '342034', '342035']
    nosy_count = 10.0
    nosy_names = ['skip.montanaro', 'rhettinger', 'vstinner', 'r.david.murray', 'SilentGhost', 'berker.peksag', 'zach.ware', 'ashishnitinpatil', 'lsowen', 'mblahay']
    pr_nums = ['12306']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue27497'
    versions = ['Python 3.8']

    @lsowen
    Copy link
    Mannequin Author

    lsowen mannequin commented Jul 12, 2016

    Currently, DictWriter.writeheader() is defined like:

        def writeheader(self):
            header = dict(zip(self.fieldnames, self.fieldnames))
            self.writerow(header)

    It would be useful to have it return the value of writerow():

        def writeheader(self):
            header = dict(zip(self.fieldnames, self.fieldnames))
            return self.writerow(header)

    This would useful because:

    1. It would match the behavior of DictWriter.writerow
    2. It would enable DictWriter.writeheader to be used in within a generator function (ala https://docs.djangoproject.com/en/1.9/howto/outputting-csv/#streaming-large-csv-files)

    @lsowen lsowen mannequin added the type-feature A feature request or enhancement label Jul 12, 2016
    @zware
    Copy link
    Member

    zware commented Jul 12, 2016

    This seems like a reasonable request, but could only be done in 3.6 as it would be a new feature.

    @zware zware added the easy label Jul 12, 2016
    @vstinner
    Copy link
    Member

    It doesn't seem right to me.

    The writer should be blocking: write *all* data, don't use partial write.

    Supporting partial write (non-blocking files) requires more changes, and it isn't worth it.

    Here the problem is that the function doesn't support partial write. Each time, it tries to write the full content.

    What is your use case?

    @bitdancer
    Copy link
    Member

    It isn't documented that writer.writeline returns anything, but what it actually returns is whatever the write method of the underlying file object returns. Obviously (given the linked example), this fact is being used. There is value in consistency, so I think we should make this change.

    @rhettinger
    Copy link
    Contributor

    It is a bit irritating to have this small API inconsistency, but I'm a little wary of propagating this undocumented and untested behavior especially given Victor's concern about whether it makes any sense in this context.

    Skip, what do you think?

    @bitdancer
    Copy link
    Member

    It doesn't make sense when the return value is that provided by io.write. It does make sense in the context of the linked example (a psuedo-file object). However, it *is* an undocumented API, even if people are using it.

    @vstinner
    Copy link
    Member

    Oh. I missed the django recipe to implement "streaming csv". It uses an
    Echo object, its write() method returns its parameter.

    This recipe looks like an abuse of the API, but I understand that there is
    a need for a kind of "partial write": give control to I/O to the caller.

    Maybe we should extend the API?

    @berkerpeksag
    Copy link
    Member

    By the way, there is an open ticket about changing the recipe in Django documentation: https://code.djangoproject.com/ticket/26040

    @smontanaro
    Copy link
    Contributor

    I agree writeheader() should have returned the value of writerow(), but have no opinion on the more esoteric stuff you're discussing. I think you could argue that adding the "return" would be a bug fix.

    Personally, I long ago got in the habit of using the writerow(dict(zip(fh, fh))) idiom, didn't catch on at the time that writeheader() was even available in 2.7, and still just use the idiom most of the time. It's a one-liner. The OP could easily use that in contexts where writeheader() doesn't do the right thing.

    @lsowen
    Copy link
    Mannequin Author

    lsowen mannequin commented Jul 14, 2016

    @berker.peksag: Good catch, thank you. In my code base I'm using a slightly different implementation which does use an iterator, so I didn't even catch that the default django example was incorrect.

    @skip.montanaro: That is exactly the one-liner I'm currently using. Thank you for documenting it here in case anyone else is looking for the same.

    @vstinner
    Copy link
    Member

    I'm now ok to add the return, but only in Python 3.6. It would be a minor new "feature".

    @ashishnitinpatil
    Copy link
    Mannequin

    ashishnitinpatil mannequin commented Sep 4, 2016

    Hi, I have added the "return" to the writeheader method. I ran the tests for the csv library (./python -m test -v test_csv) and got no errors (Ran 101 tests in 0.322s; 1 test OK.).

    Kindly review the patch.

    @ashishnitinpatil
    Copy link
    Mannequin

    ashishnitinpatil mannequin commented Sep 4, 2016

    Also, I noticed in the documentation for csv.DictWriter (https://docs.python.org/3/library/csv.html#csv.DictWriter.writeheader) does not contain the following line -
    to the writer's file object, formatted according to the current dialect.
    Although it is there for the other 2 methods (writerow & writerows) and since writeheader uses writerow in turn, I believe the said line should be included for the writeheader documentation as well. I have created another patch with both the changes. I ran the Doc build (make html) and have checked the addition of the said lines looks good & does not break anything.

    I can make another thread if required, if you feel both the changes can't be accommodated in the same issue.

    @berkerpeksag
    Copy link
    Member

    Thanks! The patch lacks a test case (you can add it into Lib/test/test_csv.py) and a versionchanged notice in documentation (see http://cpython-devguide.readthedocs.io/en/latest/documenting.html#paragraph-level-markup for details.)

    @smontanaro
    Copy link
    Contributor

    Also, a change in behavior has to be very carefully considered, even
    though it passes all existing test cases (and presumably the one(s)
    you'll add). The change may well only be acceptable for the next minor
    release of 3.x.

    On Mon, Sep 5, 2016 at 7:31 AM, Berker Peksag <report@bugs.python.org> wrote:

    Berker Peksag added the comment:

    Thanks! The patch lacks a test case (you can add it into Lib/test/test_csv.py) and a versionchanged notice in documentation (see http://cpython-devguide.readthedocs.io/en/latest/documenting.html#paragraph-level-markup for details.)

    ----------
    stage: needs patch -> patch review


    Python tracker <report@bugs.python.org>
    <http://bugs.python.org/issue27497\>


    @ashishnitinpatil
    Copy link
    Mannequin

    ashishnitinpatil mannequin commented Sep 5, 2016

    Thank you Berker & Skip for your review & additional inputs.
    I have added a (probably not so good) unittest for testing the newly added *return*.
    I have also added the corresponding *versionchanged* info to the documentation as suggested by Berker.

    Will keep these in mind while making a patch next time :)
    Kindly let me know if there are any further changes required.

    @remilapeyre remilapeyre mannequin added the 3.8 only security fixes label Mar 13, 2019
    @mblahay
    Copy link
    Mannequin

    mblahay mannequin commented May 6, 2019

    I would like to drive this to conclusion since it appears this issue has not had any real activity in a while. First thing that needs to be determined is whether this enhancement is still desirable. Who can answer this?

    @SilentGhost
    Copy link
    Mannequin

    SilentGhost mannequin commented May 6, 2019

    Michael, there is a up-to-date PR that's waiting for approval.

    @mblahay
    Copy link
    Mannequin

    mblahay mannequin commented May 6, 2019

    Ah ha, so there is. I'm glad this one will get closed out. Sorry for noob mistake.

    @smontanaro
    Copy link
    Contributor

    I think this is ready to go. I added a comment to PR12306. As I am no longer a committer, I'm not sure what the next step is.

    @vstinner
    Copy link
    Member

    New changeset fce5ff1 by Victor Stinner (Rémi Lapeyre) in branch 'master':
    bpo-27497: Add return value to csv.DictWriter.writeheader (GH-12306)
    fce5ff1

    @vstinner
    Copy link
    Member

    I merged PR 12306. Thanks Ashish Nitin Patil for the initial patch, thanks Rémi Lapeyre to converting it to a PR, thanks Logan for the initial bug report ;-)

    Sadly, this change is a new feature and so cannot be backported to 2.7 or 3.7. The workaround is override the method:

        def writeheader(self):
            header = dict(zip(self.fieldnames, self.fieldnames))
            return self.writerow(header)

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.8 only security fixes easy type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    6 participants