← 返回首页
bpo-34533: Apply PEP384 to _csv module by eduardo-elizondo · Pull Request #8977 · python/cpython · GitHub
Skip to content

Navigation Menu

Toggle navigation
Sign in
Appearance settings
Search or jump to...

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Include my email address so I can be contacted
Cancel Submit feedback

Saved searches

Use saved searches to filter your results more quickly

Cancel Create saved search
Appearance settings
Resetting focus

bpo-34533: Apply PEP384 to _csv module#8977

Closed
eduardo-elizondo wants to merge 8 commits into
python:masterfrom
eduardo-elizondo:csv-fromspec
Closed

bpo-34533: Apply PEP384 to _csv module#8977
eduardo-elizondo wants to merge 8 commits into
python:masterfrom
eduardo-elizondo:csv-fromspec

Conversation

eduardo-elizondo commented Aug 28, 2018 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Contributor

ZackerySpytz commented Aug 28, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor

You should ensure the build actually succeeds and the tests pass before you submit a PR (see also the developer's guide).

Be aware that I'm preparing to submit a PR applying this refactoring to the _ssl module (based on the old patch at https://bugs.python.org/issue15670).

Also, the issue that you created on the tracker happens to be a duplicate of https://bugs.python.org/issue14935.

eduardo-elizondo commented Aug 28, 2018

Copy link
Copy Markdown
Contributor Author

@ZackerySpytz Sorry about that, contbuild is passing now!

There was an incorrect assumption on one of the csv modules test. Given that Dialect class is a HeapType now, it should be able to be pickled when proto == 0 or 1.

Also, good point. I didn't see that previous issue. Given that the other one has been there for 6+ years, we can probably just take this one :)

shanxS reviewed Aug 29, 2018
View reviewed changes
Comment thread Modules/_csv.c Outdated

static struct PyModuleDef _csvmodule;
static struct PyModuleDef _csvmodule_def;
static PyObject *_csvmodule;

shanxS Aug 29, 2018

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

Do we need this at file scope? Given that this is replacing PyObject *module at line 1618

eduardo-elizondo Aug 29, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

_csvmodule_def is required in _csvstate_global. But you are right, we don't need _csvmodule at file scope. Will fix.

tiran requested a review from DinoV September 11, 2019 17:19
eduardo-elizondo mentioned this pull request Sep 13, 2019
Closed
erlend-aasland mentioned this pull request May 7, 2023
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

DinoV Awaiting requested review from DinoV
1 more reviewer
shanxS shanxS left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

DinoV

Labels

awaiting review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

Footer

© 2026 GitHub, Inc.