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 #1233

Merged
merged 4 commits into from
Jan 14, 2022
Merged

Add slotscheck #1233

merged 4 commits into from
Jan 14, 2022

Conversation

ariebovenberg
Copy link
Contributor

I have made things!

@sobolevn kindly invited me to add slotscheck to the CI.

Checklist

  • I have double checked that there are no unrelated changes in this pull request (old patches, accidental config files, etc)
  • I have created at least one test case for the changes I have made n/a
  • I have updated the documentation for the changes I have made
  • I have added my changes to the CHANGELOG.md

Related issues

related: #1147, #1144

Open questions

  • I've kept the same slots rules as present in test_slots.py. This means returns.contrib.mypy is not checked. We can either:
    a) add slots to mypy plugin classes
    b) keep the current ignore
    c) run slotscheck on this part of the code with more relaxed settings (there is an issue open multi-setting support if you have an opinion)
  • The UnwrapFailedError class reports an error. This is because BaseException actually has a __dict__. We can either:
    a) remove slots from UnwrapFailedError
    b) keep the current ignore

Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

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

Thanks!

@@ -73,6 +80,7 @@ flake8-pyi = "^20.10"
# correctly with Python 3.10. That lib is a dependency of `nitpick`!
nitpick = { version = "^0.29", python = "<3.10" }
codespell = "^2.1"
slotscheck = "^0.5.1"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
slotscheck = "^0.5.1"
slotscheck = "^0.5"

require-subclass = true
require-superclass = true
exclude-modules = 'returns\.contrib\.mypy'
exclude-classes = 'returns\.primitives\.exceptions:UnwrapFailedError'
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@ariebovenberg ariebovenberg Jan 14, 2022

Choose a reason for hiding this comment

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

BaseException (Python stdlib) doesn't have slots (see questions in 'open questions' in top comment)

@sobolevn
Copy link
Member

I've kept the same slots rules as present in test_slots.py. This means returns.contrib.mypy is not checked. We can either:

Let's go with b. No need to check mypy plugin for slots.

The UnwrapFailedError class reports an error. This is because BaseException actually has a dict. We can either:

I think having a __slot__ definition is still makes attr access a bit faster, doesn't it? If so, I am happy with special ignoring it.

@ariebovenberg
Copy link
Contributor Author

  • Indeed there is still an advantage to slots, even if the base class doesn't support it. It may be common enough to add a --no-require-exceptions flag to slotscheck to disable slots check for exception subclasses.
  • any idea what the docs error is about? (Before I start debugging locally)

@codecov
Copy link

codecov bot commented Jan 14, 2022

Codecov Report

Merging #1233 (99e954d) into master (af08071) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##            master     #1233   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           79        79           
  Lines         2434      2434           
  Branches       211       211           
=========================================
  Hits          2434      2434           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update af08071...99e954d. Read the comment docs.

@sobolevn
Copy link
Member

@ariebovenberg it is unrelated, I broke it recently somehow (sphinx 😒 ).

Fix in a separate PR is highly appreciated.

Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

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

Thanks again!

@sobolevn sobolevn merged commit 08dfbed into dry-python:master Jan 14, 2022
@ariebovenberg
Copy link
Contributor Author

@ariebovenberg it is unrelated, I broke it recently somehow (sphinx 😒 ).

Fix in a separate PR is highly appreciated.

An issue is a start: #1234

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

Successfully merging this pull request may close these issues.

2 participants