gh-114123: migrate docstring from _csv to csv

[smontanaro]

• Issue: csv module docstring imported from _csv module #114123

[smontanaro]

I don't think this requires a News entry. In theory, the _csv module is an implementation detail. Nothing changed when asking for csv.__doc__.

[smontanaro]

Not sure who the default documentation reviewer is. I suspect @encukou will know...

[AA-Turner]

This seems fine but I'm not a csv expert!

A

[AA-Turner]

Thoughts on backports? I'm somewhat tempted, as future changes to the CSV docstring will be much harder if this PR isn't backported.

A

[smontanaro]

Thoughts on backports? I'm somewhat tempted, as future changes to the CSV docstring will be much harder if this PR isn't backported.

I don't really have an opinion on the topic. If it simplifies future edits (are doc changes common in older versions?) I guess it's fine. That said, I also have no idea what's involved to backport the change.

…

[serhiy-storchaka]

I think that this change needs a NEWS entry, because it has a user visible effect in public module (for example removing "__doc__" from __all__).

And while we are here, why not remove also "__version__" from __all__?

[smontanaro]

@serhiy-storchaka Thanks, I'll take care of both later today.

[erlend-aasland]

@serhiy-storchaka, some code may rely on __version__. I think this PR should focus on the docstring only. See previous more or less relevant discussions:

• https://mail.python.org/archives/list/[email protected]/thread/KBU4EU2JULXSMUZULD5HJJWCGOMN52MK/#W2VCBOOS7NSUMEDBO7UOB22Z3CBVZIV5
• Misleading __version__ attribute of modules in standard library #76007
• Deprecate sqlite3.version and sqlite3.version_info #93370

[smontanaro]

I didn't see @erlend-aasland's note about creeping __version__ism. That said, I'm happy to remove it from the csv module altogether if that's what folks want. Maybe someone relies on it, but I'd be kinda surprised. In any case, it doesn't belong in __all__ (IMO), and there's no need to define it in C code just to import it into the front-end Python module.

Removing __version__ shouldn't be backported though. If removing it is the path forward, I should probably do it in a second PR which isn't going to be backported.

[serhiy-storchaka]

LGTM, besides niypicks about a NEWS file.

[AA-Turner]

I think test__all__ should be kept?

Suggested change

	def test__all__(self):
	extra = {'__doc__', '__version__'}
	support.check__all__(self, csv, ('csv', '_csv'), extra=extra)
	def test__all__(self):
	support.check__all__(self, csv, {'csv', '_csv'})

[smontanaro]

@AA-Turner Isn't test__all__ a standard thing? Why is this override needed if we no longer have an extra?

[AA-Turner]

test.support.check__all__() is, but test__all__ isn't defined in unittest.TestCase, so we do need to define the test to keep it.

[serhiy-storchaka]

No, it is not overridden, it is added. It compares the __all__ content with the calculated expected value (all public function, classes, etc defined in the specified modules). extra is an extra.

[smontanaro]

Before anybody merges (not my job), let's settle on the fate of csv.__version__.

[AA-Turner]

No objection to removing csv.__version__, but I think it should be done in a new PR with a proper deprecation, which wouldn't block this PR's goal of moving __version__ to csv.py (and indeed, having __version__ defined in Python would make the deprecation mildly easier).

_deprecated___version__ = "1.0"

def __getattr__(name):
    if name == "__version__":
        from warnings import _deprecated

        _deprecated(name, remove=(3, 15))
        return globals()[f"_deprecated_{name}"]
    raise AttributeError(f"module {__name__!r} has no attribute {name!r}")

A

[erlend-aasland]

The fate of __version__ lies in another issue and PR. We can use #76007 for the issue, as csv is already mentioned there.

[smontanaro]

No objection to removing csv.__version__, but I think it should be done in a new PR with a proper deprecation, which wouldn't block this PR's goal of moving __version__ to csv.py (and indeed, having __version__ defined in Python would make the deprecation mildly easier).

_deprecated___version__ = "1.0"

def __getattr__(name):
    if name == "__version__":
        from warnings import _deprecated

        _deprecated(name, remove=(3, 15))
        return globals()[f"_deprecated_{name}"]
    raise AttributeError(f"module {__name__!r} has no attribute {name!r}")

A

Is this the standard idiom for deprecating names?

[erlend-aasland]

Is this the standard idiom for deprecating names?

That pattern (and variations over it) is used several places in Lib/, so I guess you can call it a standard idiom.

[merwok]

Suggested change

	of Comma Separated Value (CSV) files, and implements the interface
	of comma-separated values files, and implements the interface

[smontanaro]

Is this the standard idiom for deprecating names? That pattern (and variations over it) is used several places in Lib/, so I guess you can call it a standard idiom.

Any possibility of standardizing it enough to stash it somewhere in the std lib? (Just thinking out loud. After a few times, it seems like it would be useful to make it into a battery.)

…

[merwok]

I am not sure that there’s enough demand for it, nor a natural place.
It’s too specific to the Python stdlib to be in warnings, and I don’t think there’s any module concerned with meta-stdlib utilities.

[miss-islington-app]

Thanks @smontanaro for the PR, and @AA-Turner for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11, 3.12.
🐍🍒⛏🤖

[miss-islington-app]

Sorry, @smontanaro and @AA-Turner, I could not cleanly backport this to 3.12 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 72abb8c5d487ead9eb115fec8132ccef5ba189e5 3.12

[miss-islington-app]

Sorry, @smontanaro and @AA-Turner, I could not cleanly backport this to 3.11 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 72abb8c5d487ead9eb115fec8132ccef5ba189e5 3.11

[smontanaro]

I don't think this change should be backported. How did the attempt happen?

[AA-Turner]

I added the backport labels after my note above:

Thoughts on backports? I'm somewhat tempted, as future changes to the CSV docstring will be much harder if this PR isn't backported

Though given the backport would require manual work regardless, I agree on leaving it for now & not backporting.

A

[smontanaro]

Hmmm... Okay, go for it. Let the chips fall where they may.

[merwok]

Even if this is all about internal location of attributes, it feels safer to not backport.
Not worth any risk, and possible docstring backports will have to be done by hand.