adodbapi: Remove references to outdated IronPython #2049
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@vernondcole WDYT? |
vernondcole
reviewed
Apr 24, 2023
vernondcole
reviewed
Apr 24, 2023
|
This work is great! -- I have been meaning to do something like this for
years, but, since I work mostly on Linux these days, have not gotten around
to it.
It needs to be a bit more aggressive than you have been in pulling old
stuff. I am adding comments as I review.
Most importantly, though, this will alter enough things that it should have
a complete test before it is merged.
I will spin up three VMs to test against, and that will take a bit of time.
The CI test does not do significant testing of this module.
…On Mon, Apr 24, 2023 at 12:24 PM Avasam ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In adodbapi/adodbapi.py
<#2049 (comment)>:
> @@ -24,7 +24,6 @@
DB-API 2.0 specification: http://www.python.org/dev/peps/pep-0249/
This module source should run correctly in CPython versions 2.7 and later,
I'm trying to keep specifically CPython2 code cleanup to its own PR (#1990
<#1990>), and splitting things up
in smaller bites. Otherwise I end up with a ton of changes if I
remove/update python2 specific code (ie, that PR), and when something break
(see failing test) it's harder to find which change did it. Comments like
these also help find said obsolete code.
That said, this comment doesn't point to specific Python2 code, so if you
want me to update it in this here PR, I can do that. (but at least wanted
to explain why I didn't remove it in the first place)
—
Reply to this email directly, view it on GitHub
<#2049 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAEZOBP6ZHPIIL7EIAMDAZTXC3AOTANCNFSM6AAAAAAXGJVJPY>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
vernondcole
reviewed
Apr 25, 2023
vernondcole
reviewed
Apr 25, 2023
vernondcole
reviewed
Apr 25, 2023
vernondcole
reviewed
Apr 25, 2023
vernondcole
reviewed
Apr 25, 2023
vernondcole
reviewed
Apr 25, 2023
vernondcole
reviewed
Apr 25, 2023
and simplify more code post IronPython logic removal
|
@vernondcole I believe all comments have been addressed |
This was referenced Jul 23, 2023
mhammond
approved these changes
Jul 25, 2023
|
oops, I mistook one of the commits for the entire PR - this still needs Vernon's review. |
f3525a4 to
838ee64
Compare
vernondcole
approved these changes
Mar 25, 2024
|
Thanks for the review! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Split off from #1990 to make it easier to review in isolation.
Remove obsoleted IronPython code and references to IronPython, which does not support any recent enough python version.