Skip to content

Windows Handles: Refactor plugin #1786

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

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

dgmcdona
Copy link
Contributor

A lot of functionality in the Handles plugin class was using instance methods instead of classmethods. This refactors the plugin to use classmethods instead of instance methods for consistency with the rest of the framework, and bumps the plugin major version to 4.0.0. It also updates plugins that depend on Handles with the correct required major version, and converts instances where an instance of the plugin is being constructed to uses of the new classmethods.

Also improves some docstrings and type-hinting in handles.py.

A lot of functionality in this plugin class was using instance methods
instead of classmethods; this refactors the plugin to use classmethods
instead of instance methods for consistency with the rest of the
framework, and bumps the plugin major version to 4.0.0.
This updates the plugins that depend on `Handles` with the correct
required version number, as well as calls to the new handles
classmethods where needed.
Copy link
Member

@ikelos ikelos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this looks a lot cleaner. Just mulling over whether the LEVEL_MASK should qualify as a fully fledged constant or not... I'm not sure it needs to live elsewhere, but I don't want people seeing it and using it to justify local constants... 5:S

self._type_map = None
self._cookie = None
self._level_mask = 7
LEVEL_MASK = 7
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't decide whether this is a constant that other plugins might want to access, or just an internal constant only for use by the plugin? What do you think?

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