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

Relax DictWriter.fieldnames closer to implementation #5994

Merged
merged 2 commits into from
Sep 2, 2021

Conversation

PeterJCLaw
Copy link
Contributor

I think this might actually work if you pass just an Iterable, however since that would allow passing a generator (which would definitely not work correctly) I've kept this as Collection.

Fixes #5993

I think this might actually work if you pass just an Iterable,
however since that would allow passing a generator (which would
definitely not work correctly) I've kept this as Collection.

Fix python#5993
@PeterJCLaw PeterJCLaw marked this pull request as draft September 1, 2021 21:52
@github-actions

This comment has been minimized.

@PeterJCLaw PeterJCLaw marked this pull request as ready for review September 2, 2021 07:05
stdlib/csv.pyi Outdated
@@ -17,7 +17,7 @@ from _csv import (
unregister_dialect as unregister_dialect,
writer as writer,
)
from typing import Any, Generic, Iterable, Iterator, Mapping, Sequence, Type, TypeVar, overload
from typing import Any, Collection, Generic, Iterable, Iterator, Mapping, Sequence, Type, TypeVar, overload
Copy link
Collaborator

@srittau srittau Sep 2, 2021

Choose a reason for hiding this comment

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

One minor thing: We should import Collection, Iterable, Iterator, Mapping, and Sequence from collections.abc instead of typing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, should that be changed here, or would it be preferable to do that separately? (I'd be happy to put together a subsequent PR which does these all together)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Whatever works best for you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

f8af7bc hopefully fixes this.

Just a thought -- would it be useful to have a flake8 plugin (custom to this repo) which checked for this? I suspect it would be fairly easy to create one and might make this preference more visible. If I recall correctly no-one would need to install anything extra, it would just be a file somewhere in the repo and some configuration in the pyproject.toml and flake8 would do the rest.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator

@Akuli Akuli left a comment

Choose a reason for hiding this comment

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

Consider also changing DictReader, so that it is consistent with DictWriter.

@PeterJCLaw
Copy link
Contributor Author

Consider also changing DictReader, so that it is consistent with DictWriter.

Hrm, I'd not spotted that you can pass fieldnames to DictReader. It seems it can indeed be a Collection (for example passing a set works), though not merely an Iterable.

However it's optional and you can omit them and get back a List from the property if you let them be read from the file.

Additionally passing a value is complicated by the fact that it's the passed value you get back from fieldnames, rather than any inferred value.

It feels like the right signature for DictReader.fieldnames therefore depends on the constructor used (and is arguably itself generic), with the default value being List[_T]. I'm not sure how to spell the generic-with-default and have a recollection that the last time I tried it wasn't supported by mypy (python/mypy#3737 looks familiar, though maybe not quite the same as this would be).

Very happy to be guided on how to spell the signature for DictReader's __init__ and fieldnames here though!

@github-actions
Copy link
Contributor

github-actions bot commented Sep 2, 2021

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

@srittau srittau merged commit 425a353 into python:master Sep 2, 2021
@srittau
Copy link
Collaborator

srittau commented Sep 2, 2021

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Should csv.DictWriter require fieldnames to be a Sequence?
3 participants