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

Add slotscheck #14

Merged
merged 15 commits into from Dec 30, 2022
Merged

Add slotscheck #14

merged 15 commits into from Dec 30, 2022

Conversation

ItsDrike
Copy link
Member

@ItsDrike ItsDrike commented Dec 29, 2022

Currently, the only thing enforcing __slots__ in the code-base is the RequiredParamsABCMixin class, which is only checking for presence of the variable and prevents initialization if this presence check fails (similarly to how ABCs work).

However there's a lot more things that we should be enforcing when it comes to __slots__, such as avoiding duplicates, ensuring all subclasses have __slots__ defined, etc. For these additional checks, we can use the slotscheck library.

Along with that, slotscheck can also require all classes to be slotted by default, and produce a linter error otherwise. Of course, we can always add specific ignores if some class shouldn't be slotted (though slotscheck is actually pretty good at recognizing those cases itself, so it might not even be that necessary).

However since slotscheck works by actually running the code it's checking, and performing it's checks on runtime, some of it's edge case ignores that it provides require proper typing imports (like to ignore the need for __slots__ in TypedDict or Protocol classes), which is why this PR also converts our current way of using typing-extensions and turns it into a full runtime dependency.

This is not a huge issue, as it seems that one of our dependencies already requires typing-extensions on runtime, so us requiring it too doesn't actually change anything about whether the library users will end up installing it.

@ItsDrike ItsDrike added t: bug Something isn't working p: 2 - normal Normal priority t: feature New request or feature a: CI Related to continuous integration and deployment a: internal Related to internal API of the project labels Dec 29, 2022
@ItsDrike ItsDrike merged commit 785b926 into main Dec 30, 2022
@ItsDrike ItsDrike deleted the add-slotscheck branch December 30, 2022 00:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a: CI Related to continuous integration and deployment a: internal Related to internal API of the project p: 2 - normal Normal priority t: bug Something isn't working t: feature New request or feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant