-
Notifications
You must be signed in to change notification settings - Fork 789
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove unused error aliases by assignment #2269
Remove unused error aliases by assignment #2269
Conversation
…d-error-aliases-by-assignement
…d-error-aliases-by-assignement
CHANGES.txt
Outdated
@@ -15,6 +15,16 @@ Coming in build 307, as yet unreleased | |||
-------------------------------------- | |||
|
|||
### pywin32 | |||
* Removed the following unused error names and aliases. (#2269, @Avasam) | |||
Here's their replacements if you were using them: | |||
* `win32com.universal.com_error` --> `pythoncom.com_error` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should soften this message a little - IIUC we don't expect any breakage here because strings as exceptions simply don't work, but a quick read implies something like "if you were catching these exceptions, here are the new names you should catch"
For public symbols which were actually errors I see no reason to stop exporting them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Certainly! I'll think how I wanna reword this.
@@ -6,8 +6,6 @@ | |||
import win32evtlog | |||
import winerror | |||
|
|||
error = win32api.error # The error the evtlog module raises. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see no reason at all to stop exporting this - it seems perfectly reasonable people would use this module directly and catch this error directly. Ditto for the others which are similar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this is re-exporting pywintypes.error
in case someone wants to use the win32evtlogutil
module without having to import pywintypes
directly.
I do have some questions though, and a bit of concern about user confusion. Which I've heavily felt when starting to use pywin32 (it's what kickstarted this whole annotations and stubs thing ^^"). Relevant here is trying to understand what's a re-export vs a different Exception class, where one should import from vs a leaked import, etc... Having a bunch of same-named aliases error
for different exception types pywintypes.error
, com_error
, TLBrowserException
doesn't help either (and can easily lead to accidental shadowing).
Does com/win32com/universal.py
really share that concern as modules in Lib
? If so, why aren't all other modules that can raise uncaught com_error
or COMException
also re-exporting them?
What about error = RuntimeError
in win32/Lib/win32serviceutil.py
though? I really don't see the point of that one. If you wanna catch RuntimeError
just use RuntimeError
no? Is it promising that absolutely no other exception types can bubble up ?
If it was its own exception class that can be differentiated that would make sense, but as a direct alias I'm not so sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are 2 questions here I think:
- Should, eg, win32serviceutil have re-exported RuntimeError as
win32serviceutil.error
- that's an interesting question and I'm really not sure of the answer. I think that a module like the above should be clear about what exceptions it throws so consumers know what to catch, and I think that:
try:
win32serviceutil.something()
except win32serviceutil.error:
...
is an improvement over catching RuntimeError. Maybe it should not have been an alias and instead should have been a real exception subclass? Maybe it should have been a different alias? Regardless of the answers to them, I think the code snippet above is clear.
- Is the more relevant question here though: even if we agree that the above point was a historical mistake, was it bad enough we want to break existing code? IMO, the answer to this is a resounding "no".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated! And added a comment clarifying that a re-export is definitely the intent.
…d-error-aliases-by-assignement
Just like #2268 , this doesn't focus on a fix for #1902 yet.
These are the aliases that are easy to remove because they are unused and/or simple aliases.
Now the question is:
Should they still be accessible with a deprecation warning ? Or is it ok to remove them?