Skip to content
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

[_comobject.py] Use WinDLL/OleDLL instead of windll/oledll #754

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

moi15moi
Copy link
Contributor

See #735

@junkmd
Copy link
Collaborator

junkmd commented Jan 21, 2025

This change requires a bit more consideration.

For example, names like kernel32 do not involve both ole and win being used together within this module, so I think there’s no need for a redundant suffix like ..._from_windll. Simply naming it _kernel32 should suffice.

This package already has existing aliases such as windll.ole32 and oledll.ole32. Following this naming convention would likely align better with the established practices of this package.

# Initialization and shutdown
_ole32 = oledll.ole32
_ole32_nohresult = windll.ole32 # use this for functions that don't return a HRESULT

Additionally, the _comobject module has so far avoided importing symbols from automation at the top level.
7f9a73d

# so we don't have to import comtypes.automation
DISPATCH_METHOD = 1
DISPATCH_PROPERTYGET = 2
DISPATCH_PROPERTYPUT = 4
DISPATCH_PROPERTYPUTREF = 8

As for importing symbols for assignment to _argtypes_, I’m uncertain about the potential impact in areas that might not be covered by the tests.

I believe the concept of this change is sufficiently useful.
However, at the initial stage, we’d prefer to start with simpler replacements that have minimal impact, and require less consideration.

For example, changes to a module like server/localserver.py, where only oledll is replaced and symbols are imported exclusively from standard libraries, would be much easier to approve.

This PR can remain open, but if you submit another PR with changes to a module involving fewer replacements, that PR is likely to be merged first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants