search

mhammond / pywin32

Python for Windows (pywin32) Extensions
4.92k stars 786 forks source link

adodbapi: Cleanup obsolete and unsupported python code #2094

Open Avasam opened 11 months ago

Avasam commented 11 months ago

Following a general understanding that adodbapi code changes will be a lot harder to test, and comments in #1990, these changes have been completely split off. With the final goal to make basic type-checking validation of public methods possible, easing the addition of 3.7+ type annotations. I am fine if that excludes adodbapi.

A handful of PRs listed below have overlapping or extracted changes. I'd recommend reviewing those first, in the mean time I'm marking this PR as draft.

This cleans-up most unreachable code. Mostly due to unsupported python versions.

vernondcole commented 11 months ago

The simplest answer here would be to delete the entire "remote" directory. I don't think that the facility has ever been used by anyone other than myself, and the use case for that is now very outdated.

I am in favor of dropping the entire subsystem as deprecated.

On Sun, Aug 6, 2023 at 10:22 AM Avasam @.***> wrote:

@.**** commented on this pull request.

In adodbapi/remote/server.py https://github.com/mhammond/pywin32/pull/2094#discussion_r1285238410:

@@ -97,7 +92,7 @@ Pyro4.config.PREFER_IP_VERSION = 0 # allow system to prefer IPv6 Pyro4.config.SERIALIZERS_ACCEPTED = set( ["serpent", "pickle"] -) # change when Py2.5 retired +) # TODO: change when Py2.5 retired

What's the change needed here ?

— Reply to this email directly, view it on GitHub https://github.com/mhammond/pywin32/pull/2094#pullrequestreview-1564174948, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAEZOBMPAGJVDZW2HGYFXDTXT7ADDANCNFSM6AAAAAA3GAEVSE . You are receiving this because you are subscribed to this thread.Message ID: @.***>

vernondcole commented 11 months ago

Yes, adodbapi/remote.py should be removed also.

On Wed, Aug 9, 2023 at 1:45 PM Avasam @.***> wrote:

@.**** commented on this pull request.

In adodbapi/remote/server.py https://github.com/mhammond/pywin32/pull/2094#discussion_r1289119382:

@@ -97,7 +92,7 @@ Pyro4.config.PREFER_IP_VERSION = 0 # allow system to prefer IPv6 Pyro4.config.SERIALIZERS_ACCEPTED = set( ["serpent", "pickle"] -) # change when Py2.5 retired +) # TODO: change when Py2.5 retired

The simplest answer here would be to delete the entire "remote" directory. I don't think that the facility has ever been used by anyone other than myself, and the use case for that is now very outdated.

A quick search I didn't find any reference to the remote directory either. I did find some to the remote module though, but only in tests.

I am in favor of dropping the entire subsystem as deprecated.

Do you also mean adodbapi/remote.py ? (I've left it in for now since you only mentioned deleting the directory)

— Reply to this email directly, view it on GitHub https://github.com/mhammond/pywin32/pull/2094#discussion_r1289119382, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAEZOBOOPTWF4ZKGKCPZYVLXUPSHNANCNFSM6AAAAAA3GAEVSE . You are receiving this because you commented.Message ID: @.***>

Avasam commented 3 months ago

@vernondcole This is a "too big to review" PR that has been split up into smaller chunks in other PRs. It contains all of the code modernization and old code removal changes at once. Useful for me to reference when there will undoubtedly be merge conflicts, and allows me to test the interaction of different changes in their final combined state.

Hence it's marked as Draft. Once everything else is merged I'll either Publish this PR or keep splitting off what's left in more reviewable chunks.

As per the PR description:

A handful of PRs listed below have overlapping or extracted changes. I'd recommend reviewing those first, in the mean time I'm marking this PR as draft.

vernondcole commented 3 months ago

Understood. I am still working on creating a set of VMs so that I can properly test this and get everything merged back in.

Avasam commented 2 months ago

@vernondcole This is now in a state ready to be reviewed. The bulk has been extracted in other PRs.