Skip to content

Remove unused error aliases by assignment #2269

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.

Sign up for GitHub

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

Merged
Avasam merged 8 commits into from
Jun 4, 2024
Merged

Remove unused error aliases by assignment #2269

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
30b9bf5
Remove unused error aliases by assignment
Avasam May 28, 2024
dfcbec8
Add changelog
Avasam May 28, 2024
43619f7
Add win32serviceutil
Avasam May 28, 2024
c16db3c
Merge branch 'main' of https://github.com/mhammond/pywin32 into unuse…
Avasam May 29, 2024
4c312a2
Merge branch 'main' of https://github.com/mhammond/pywin32 into unuse…
Avasam May 29, 2024
895040f
Address PR comments
Avasam May 31, 2024
ca6d4cb
Merge branch 'main' of https://github.com/mhammond/pywin32 into unuse…
Avasam Jun 4, 2024
20fa156
Merge branch 'main' into unused-error-aliases-by-assignement
Avasam Jun 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions CHANGES.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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`
Copy link
Copy Markdown
Owner

@mhammond mhammond May 29, 2024

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.

Copy link
Copy Markdown
Collaborator Author

@Avasam Avasam May 29, 2024

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.

* `win32com.client.build.error` --> `"PythonCOM.Client.Build error"`
* `win32com.client.genpy.error` --> `"makepy.error"`
* `win32com.client.tlbrowse.error` --> `win32com.client.tlbrowse.TLBrowserException`
* `win32serviceutil.error` --> `RuntimeError`
* `win32evtlogutil.error` -> `win32api.error` or `pywintypes.error`
* `win32pdhutil.error` -> `win32pdh.error` or `pywintypes.error`
* `sspi.error` is still importable, but no longer exposed as a public member, use `win32security.error` or `pywintypes.error` instead
* Add EnumDesktopWindows (#2219, @CristiFati)
* Marked `exc_type` and `exc_traceback` in `win32comext.axscript.client.error.AXScriptException.__init__` as deprecated.
They are now unused and all information is taken from the `exc_value` parameter.
Expand Down
2 changes: 0 additions & 2 deletions com/win32com/client/build.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,6 @@
# always render the string perfectly - so just punt and fall-back to a repr()
_makeDocString = repr

error = "PythonCOM.Client.Build error"


class NotSupportedException(Exception):
pass # Raised when we cant support a param type.
Expand Down
1 change: 0 additions & 1 deletion com/win32com/client/genpy.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@

from . import build

error = "makepy.error"
makepy_version = "0.5.01" # Written to generated file.

GEN_FULL = "full"
Expand Down
4 changes: 1 addition & 3 deletions com/win32com/client/tlbrowse.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,6 @@ class TLBrowserException(Exception):
"TypeLib browser internal error"


error = TLBrowserException

FRAMEDLG_STD = win32con.WS_CAPTION | win32con.WS_SYSMENU
SS_STD = win32con.WS_CHILD | win32con.WS_VISIBLE
BS_STD = SS_STD | win32con.WS_TABSTOP
Expand Down Expand Up @@ -208,7 +206,7 @@ def _GetRealMemberPos(self, pos):
elif pos >= 0:
return pos, 0
else:
raise error("The position is not valid")
raise TLBrowserException("The position is not valid")

def CmdMemberListbox(self, id, code):
if code == win32con.LBN_SELCHANGE:
Expand Down
17 changes: 7 additions & 10 deletions com/win32com/universal.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,6 @@
import pythoncom
from win32com.client import gencache

com_error = pythoncom.com_error
_univgw = pythoncom._univgw


def RegisterInterfaces(typelibGUID, lcid, major, minor, interface_names=None):
ret = [] # return a list of (dispid, funcname for our policy's benefit
Expand Down Expand Up @@ -86,15 +83,15 @@ def RegisterInterfaces(typelibGUID, lcid, major, minor, interface_names=None):

def _doCreateVTable(iid, interface_name, is_dispatch, method_defs):
defn = Definition(iid, is_dispatch, method_defs)
vtbl = _univgw.CreateVTable(defn, is_dispatch)
_univgw.RegisterVTable(vtbl, iid, interface_name)
vtbl = pythoncom._univgw.CreateVTable(defn, is_dispatch)
pythoncom._univgw.RegisterVTable(vtbl, iid, interface_name)


def _CalcTypeSize(typeTuple):
t = typeTuple[0]
if t & (pythoncom.VT_BYREF | pythoncom.VT_ARRAY):
# Its a pointer.
cb = _univgw.SizeOfVT(pythoncom.VT_PTR)[1]
cb = pythoncom._univgw.SizeOfVT(pythoncom.VT_PTR)[1]
elif t == pythoncom.VT_RECORD:
# Just because a type library uses records doesn't mean the user
# is trying to. We need to better place to warn about this, but it
Expand All @@ -104,10 +101,10 @@ def _CalcTypeSize(typeTuple):
# warnings.warn("warning: records are known to not work for vtable interfaces")
# except ImportError:
# print("warning: records are known to not work for vtable interfaces")
cb = _univgw.SizeOfVT(pythoncom.VT_PTR)[1]
cb = pythoncom._univgw.SizeOfVT(pythoncom.VT_PTR)[1]
# cb = typeInfo.GetTypeAttr().cbSizeInstance
else:
cb = _univgw.SizeOfVT(t)[1]
cb = pythoncom._univgw.SizeOfVT(t)[1]
return cb


Expand Down Expand Up @@ -191,8 +188,8 @@ def dispatch(
ob,
index,
argPtr,
ReadFromInTuple=_univgw.ReadFromInTuple,
WriteFromOutTuple=_univgw.WriteFromOutTuple,
ReadFromInTuple=pythoncom._univgw.ReadFromInTuple,
WriteFromOutTuple=pythoncom._univgw.WriteFromOutTuple,
):
"Dispatch a call to an interface method."
meth = self._methods[index]
Expand Down
3 changes: 1 addition & 2 deletions win32/Lib/sspi.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,7 @@
# $Id$
import sspicon
import win32security

error = win32security.error
from win32security import error


class _BaseAuth:
Expand Down
2 changes: 0 additions & 2 deletions win32/Lib/win32evtlogutil.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,6 @@
import win32evtlog
import winerror

error = win32api.error # The error the evtlog module raises.
Copy link
Copy Markdown
Owner

@mhammond mhammond May 29, 2024

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.

Copy link
Copy Markdown
Collaborator Author

@Avasam Avasam May 29, 2024

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.

Copy link
Copy Markdown
Owner

@mhammond mhammond May 29, 2024

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".

Copy link
Copy Markdown
Collaborator Author

@Avasam Avasam May 31, 2024

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.


langid = win32api.MAKELANGID(win32con.LANG_NEUTRAL, win32con.SUBLANG_NEUTRAL)


Expand Down
2 changes: 0 additions & 2 deletions win32/Lib/win32pdhutil.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,6 @@

import win32pdh

error = win32pdh.error

# Handle some localization issues.
# see http://support.microsoft.com/default.aspx?scid=http://support.microsoft.com:80/support/kb/articles/Q287/1/59.asp&NoWebContent=1
# Build a map of english_counter_name: counter_id
Expand Down
6 changes: 2 additions & 4 deletions win32/Lib/win32serviceutil.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,6 @@
import win32service
import winerror

error = RuntimeError


# Returns the full path to an executable for hosting a Python service - typically
# 'pythonservice.exe'
Expand Down Expand Up @@ -54,7 +52,7 @@ def LocatePythonServiceExe(exe=None):
win32api.CopyFile(maybe, correct)

if not os.path.exists(correct):
raise error(f"Can't find '{correct}'")
raise RuntimeError(f"Can't find '{correct}'")

# If pywintypes.dll isn't next to us, or at least next to pythonXX.dll,
# there's a good chance the service will not run. That's usually copied by
Expand Down Expand Up @@ -624,7 +622,7 @@ def GetServiceClassString(cls, argv=None):
# Get the long name
fname = os.path.join(path, filelist[0][8])
except win32api.error:
raise error(
raise RuntimeError(
"Could not resolve the path name '%s' to a full path" % (argv[0])
)
modName = os.path.splitext(fname)[0]
Expand Down