Cleanup unsupported python code (except for adodbapi) #1990
Conversation
|
We have a problem here.
Is there some facility local to the CI (or nearby) on which we can maintain, or spin up, a small SQL Server instance? (This could be an Ubuntu machine running a Linux version of Sql Server, but maintaining a Windows based example would be easier.) |
|
@vernondcole Thanks for your input! If there are currently untestable areas that would require a lot of work to setup. I can always split up those sections in a different PR so that it's tackled separately. Or let you test all at once since it'll have to be done at some point anyway. |
|
If we can come up with an expeditious testing method, I don't see a need
for a separate PR. On the other hand, if testing slows down a release
substantially, then it would be a good idea. Don't split it yet, but let's
keep the idea in mind.
I could certainly write a Salt script, for my local development
environment, to spin up three virtual machines to run the database servers
and another to build and test pywin32. But it would be a better solution to
do that in the CI environment, I suspect.
We may want to set up two levels of tests, a quirk one for automatic
go/no-go in the CI, and a more detailed one run on a periodic basis.
Testing adodbapi takes several minutes at best even when the database
servers are kept running. I don't know what our CI system is capable of,
and if someone who does wanted to start a separate conversation about this
subject, I would love it.
…On Thu, Dec 8, 2022 at 7:29 AM Avasam ***@***.***> wrote:
@vernondcole <https://github.com/vernondcole> Thanks for your input! If
there are currently untestable areas that would require a lot of work to
setup. I can always split up those sections in a different PR so that it's
tackled separately. Or let you test all at once since it'll have to be done
at some point anyway.
—
Reply to this email directly, view it on GitHub
<#1990 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAEZOBIMLKY2APU64JGGXM3WMHWDRANCNFSM6AAAAAASXDAW5A>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
There was a problem hiding this comment.
Until #1986 lands it will be tricky to look closely, but a very quick look seems great.
I think we should split adodbapi, but the question is, where: @vernondcole in #1986 there are a couple of commits which are tool generated, first of which sorts imports. How do you feel about an untested change which just does this, for example? We could split things at one of the "tool generated" steps which might make things easier - eg, I tend to let black reformat for me without bothering to re-test what it did.
Avasam
force-pushed
the
obsolete-python-code
branch
from
b9576d4
to
03400f1
Compare
Extracted from mhammond#1990
.encode on direct strings Replace uses of str2bytes
… obsolete-python-code
… obsolete-python-code
Avasam
changed the title
|
Refactoring of adodbapi code has been completely split off to #2094 so that this here PR can be testable and reviewable sooner. |
Avasam
force-pushed
the
obsolete-python-code
branch
from
41998ac
to
7510c94
Compare
Pythonwin/pywin/idle/readme.txt
Outdated
| @@ -6,7 +6,8 @@ Pythonwin. In ALL cases, the files in this directory that also appear | |||
| in the main IDLE directory should be indentical to the latest available | |||
| for IDLE. | |||
|
|
|||
| Eg, If you have Python 1.5.2 installed, the files in this | |||
| <!-- TODO: Is this still accurate? --> | |||
There was a problem hiding this comment.
I think this should be reworded to reflect something like "if the files in this directory are newer than the ones shipped with your Python version" or similar.
There was a problem hiding this comment.
I'm not sure how I would integrate that text. If they are newer, then what?
The current text seems to imply that this directory was based on IDLE from a specific python version (>3.7). What I'm not sure about, is whether saying that Python 3.8 is the version it's based on is accurate. Or if I'm even understanding this message correctly.
Also by "later" does this text mean "newer" ?
There was a problem hiding this comment.
This comment was trying to reflect that the files in this dir also exist in IDLE, but may have been released as part of pywin32 before a Python release was made. So yeah, I guess this entire readme can now just say something like "these files have been forked from IDLE" or similar.
There was a problem hiding this comment.
Ok. I have pushed a wording change suggestion that should not fall out of date.
|
|
||
| sys.modules["builtins"].input = Win32RawInput | ||
| sys.modules["builtins"].input = Win32Input |
There was a problem hiding this comment.
raw_input doesn't exist on python 3. So
import code
sys.modules["builtins"].input = Win32RawInputwas the only code that can run.
Win32Input is no longer used.
The concept of raw input vs regular input doesn't exist in python 3. So the name didn't make sense anymore. So I removed the old unused Win32Input and renamed Win32RawInput to Win32Input
There was a problem hiding this comment.
yeah, thanks, I understood the intent - I was just wondering if you actually checked that, eg, input("enter something:") from the interactive window still actually worked? Sadly I have no windows machine handy at the moment.
There was a problem hiding this comment.
Gotcha. No I had not tested this "as a user would" through the app. I've only validated my understanding of small sections.
Now I did, there you go:
| @@ -20,10 +20,10 @@ | |||
|
|
|||
| t = ts.NewWorkItem(task_name) | |||
| t.SetComment("Test a task running as local system acct") | |||
| t.SetApplicationName("c:\\python23\\python.exe") | |||
| t.SetApplicationName("c:\\Python37\\python.exe") | |||
There was a problem hiding this comment.
we should really do the same as we did for test_addtask_1.py here - do you mind opening a followup (either an issue or PR)? Not I'm going to be largely offline for 2 weeks.
There was a problem hiding this comment.
There you go: #2099
And thanks for letting me know! Getting this PR merged is a huge step forward, I can start looking at the next steps.
This is
the seconda PR in a series concerning code cleanup. With the final goal to make basic type-checking validation of public methods possible, easing the addition of 3.7+ type annotations.This cleans-up most unreachable code. Mostly due to unsupported python versions.
TODOto the comments that mentioned python 2/3 specific code. But that I didn't know how to address.Replaced use of deprecated Types(done in multiple PRs listed below)Obsolete and redundant aliasesRemove obsolete aliases except unicode-related in non-adodbapi code #2087Redundant object subclassingadodbapi: Remove redundant object subclassing #2086Obsolete references to older python unicode compatibilityRemoved redundant and obsolete references to older python unicode compatibility #2085Removed duplicate methods foundRemove duplicated definitions #2050Used bytes-string (Use bytes-string for constant bytes values #2046b"") directly instead of"".encode()when applicableUse of raw strings when pattern-matchingUse raw string for pattern matching #2045Note these changes are for pure python code only. It'd be nice to do the same for c-modules, but I don't have the necessary knowledge or expertise there. I'm sure there's more I could do, if I missed anything, please let me know!