GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-34533: Apply PEP384 to _csv module by eduardo-elizondo · Pull Request #8977 · python/cpython

Conversation

@eduardo-elizondo

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

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

https://bugs.python.org/issue34533

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@eduardo-elizondo
Apply PEP384 to _csv module
47d1f72
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Aug 28, 2018
@bedevere-bot bedevere-bot added the awaiting review label Aug 28, 2018
@eduardo-elizondo
Added News
b41e84d
@ZackerySpytz

ZackerySpytz commented Aug 28, 2018 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
eduardo-elizondo added 4 commits August 28, 2018 12:30
@eduardo-elizondo
Fixed test and improves csv file
9adcfe7
@eduardo-elizondo
Added module name
f61164a
@eduardo-elizondo
csv Fixes
22af36a
@eduardo-elizondo
Fix merge artifacts
5eb802a
@eduardo-elizondo

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 :)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
shanxS
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 shanxS Aug 29, 2018

Copy link
Copy Markdown

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@eduardo-elizondo eduardo-elizondo Aug 29, 2018

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
eduardo-elizondo added 2 commits August 29, 2018 08:58
@eduardo-elizondo
Removed static module
c04190f
@eduardo-elizondo
Add subinterpreter support
cbd2085
@tiran
tiran requested a review from DinoV September 11, 2019 17:19
@tiran tiran assigned DinoV Sep 11, 2019
@eduardo-elizondo eduardo-elizondo mentioned this pull request Sep 13, 2019
bpo-34533: Remove all static state from _csv module #16078
Closed
@eduardo-elizondo eduardo-elizondo closed this Sep 13, 2019
@erlend-aasland erlend-aasland mentioned this pull request May 7, 2023
gh-104265 Disallow instantiation of _csv.Reader and _csv.Writer #104266
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

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

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@DinoV DinoV

Labels

awaiting review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@eduardo-elizondo @ZackerySpytz @shanxS @DinoV @the-knights-who-say-ni @bedevere-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.