GeezerVison

When you remove positional parameters, you have to make the following parameters keyword-only.

Oh right, done. I didn't document this change. Is it worth to document it?

Were similar changes documented in the past?

Please add tests that only such positional arguments are accepted. In most cases None is a valid argument both for the first deleted parameter and for the first remaining parameter, so you can just add None as an excessive positional argument.

Were similar changes documented in the past?

In commit 9baf242, I removed a buffering parameter in bz2.BZ2File. I see that I wrote ".. versionchanged:: 3.9 (...) The compresslevel parameter became keyword-only." in the doc.

Honestly, I don't think that it should be documented. I don't expect people to call such constructor with 6 parameters or more. I'm only aware of people who called CodeType() with tons of arguments: problem solved with CodeType.replace() method addition.

Why is it removed?

If you provide a SSLContext, you get different protocol than FTP_TLS.ssl_version. This removal is not required, it's more to make the API consistent with the other modified modules: the SSLContext constructor is the only place setting default parameter values. Do you prefer to keep it?

I do not know whether it is documented, but it specifies the default protocol if you do not provide an SSLContext. The user can override it in FTP_TLS or subclasses.

Who added this attribute? Ask him.

All protocols except PROTOCOL_TLS, TLS_CLIENT, and TLS_SERVER are deprecated anyways because OpenSSL is going to remove them eventually.

@tiran: Do you see a reason to keep ssl_version to specify a different "ssl version" when context parameter is omitted?

The practical problem is more that applications relying on this feature may not notice that overriden ssl_version are now ignored silently.

Note: I hate mentions of "ssl", since SSL no longer exists, it was replaced with TLS, but the Python module is still called "ssl"... as OpenSSL ;-)

There is no reason to keep the variable here. ssl.PROTOCOL_TLS_CLIENT is the only correct and fully supported value. In 3.13 I will remove all the other protocols. Only ssl.PROTOCOL_TLS_CLIENT and ssl.PROTOCOL_TLS_SERVER will stay.

I do not know whether it is documented, but it specifies the default protocol if you do not provide an SSLContext. The user can override it in FTP_TLS or subclasses.

Who added this attribute? Ask him.

If it is the right way to do, should this change be backported as a bug fix? I think it deserves a separate issue.

That's a good question. Sadly, I don't know well the ftplib module. I'm not comfortable to change it in a stable Python version. In case of doubt, I prefer to leave the code as it is in stable branches, unless someone reports a bug.

All protocols except PROTOCOL_TLS, TLS_CLIENT, and TLS_SERVER are deprecated anyways because OpenSSL is going to remove them eventually.

SSLv2 and SSLv3 are no longer supported. You can remove the AUTH SSL block.

When I read Modules/_ssl.c, it's not obvious that SSL is no longer supported. There is still conditonal code:

#ifndef OPENSSL_NO_SSL2
    PyModule_AddIntConstant(m, "PROTOCOL_SSLv2",
                            PY_SSL_VERSION_SSL2);
#endif
#ifndef OPENSSL_NO_SSL3
    PyModule_AddIntConstant(m, "PROTOCOL_SSLv3",
                            PY_SSL_VERSION_SSL3);
#endif

These constants are still documented but marked as "deprecated" in the doc: https://docs.python.org/dev/library/ssl.html#ssl.PROTOCOL_SSLv2

I would prefer to fully remove support for SSL in the _ssl/ssl modules, before changing the ftplib module.

Another example: https://docs.python.org/dev/library/ssl.html#ssl.OP_NO_SSLv3 still exists (at least in the doc).

It's documented in ssl.rst. I'm using the word "supported" in the sense of a social contract. The presence of the constants do not mean that a user can expect they are doing anything sensible.

You can remove the check. It is performed by SSLContext internally.

This line is completely redundant, because the output of pydoc already contains the constructor signature. The same in other classes in this module, and maybe in other modules.

I would prefer to remove this line and edit the following parameters descriptions, and do it before merging this PR to make backporting easier.

This PR is already big enough. If you want to enhance the documentation, please change it in a separated change. I don't see how updating this doc is a pre-requirement to remove the two parameters.