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.

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

Jump to bottom

Bug: slotscheck pre-commit misconfiguration #3251

Closed
4 tasks
ariebovenberg opened this issue Mar 25, 2024 · 2 comments
Closed
4 tasks

Bug: slotscheck pre-commit misconfiguration #3251

ariebovenberg opened this issue Mar 25, 2024 · 2 comments
Labels
Bug 🐛 This is something that is not working as expected

Comments

@ariebovenberg
Copy link
Contributor

ariebovenberg commented Mar 25, 2024 •
edited by polar-sh bot
Loading

Description

👋 Hi there @Goldziher. I'm happy to see you're still using slotscheck, but I noticed that the pre-commit exclude option is configured so that it isn't run on most files:

$ pre-commit run slotscheck --all-files -v
All OK!
stats:
  modules:     8
    checked:   8
    excluded:  0
    skipped:   5

  classes:     1
    has slots: 1
    no slots:  0
    n/a:       0

8 modules scanned is way too few, so I had a look. It took me a while to realize that the pre-commit option exclude: "test_*|docs|.github" regex matches test and litestar contains the string test 😬.

It also looks like the flake8-dunder-all pre-commit would have the same issue 👀. Might be worth scanning any other tooling you have set up to ignore things with "test" in the name 😄

I'll have a look now and, if the change is small, submit a PR shortly.

URL to code causing the issue

No response

MCVE

No response

Steps to reproduce

No response

Screenshots

No response

Logs

No response

Litestar Version

main

Platform

  • Linux
  • Mac
  • Windows
  • Other (Please specify in the description above)

Note

While we are open for sponsoring on GitHub Sponsors and
OpenCollective, we also utilize Polar.sh to engage in pledge-based sponsorship.

Check out all issues funded or available for funding on our Polar.sh dashboard

  • If you would like to see an issue prioritized, make a pledge towards it!
  • We receive the pledge once the issue is completed & verified
  • This, along with engagement in the community, helps us know which features are a priority to our users.
Fund with Polar
@ariebovenberg ariebovenberg added the Bug 🐛 This is something that is not working as expected label Mar 25, 2024
@peterschutt
Copy link
Contributor

litestar contains the string test 😬.

:D oh dear

ariebovenberg added a commit to ariebovenberg/litestar that referenced this issue Mar 27, 2024
@ariebovenberg
fix: address __slots__ and __all__ misconfigurations (litestar-org#3251)
46f0278
@ariebovenberg ariebovenberg mentioned this issue Mar 27, 2024
Closed
peterschutt added a commit that referenced this issue Mar 28, 2024
@peterschutt @ariebovenberg
ci: run slotscheck cli as ci job
4119443 
This PR moves running slotscheck from within pre-commit to its own job in ci (same as we've done with other tools that require the configured environment to run).

This partially fixes an issue identified in #3251 where the `litestar` package was inadvertently excluded from this check due to an incorrect regex pattern.

Applies fixes for issues identified by the tool.

Co-authored-by: Arie Bovenberg <a.c.bovenberg@gmail.com>
@peterschutt peterschutt mentioned this issue Mar 28, 2024
Merged
peterschutt added a commit that referenced this issue Mar 28, 2024
@peterschutt @ariebovenberg
ci: run slotscheck cli as ci job (#3275)
d2cb891 
This PR moves running slotscheck from within pre-commit to its own job in ci (same as we've done with other tools that require the configured environment to run).

This partially fixes an issue identified in #3251 where the `litestar` package was inadvertently excluded from this check due to an incorrect regex pattern.

Applies fixes for issues identified by the tool.

Co-authored-by: Arie Bovenberg <a.c.bovenberg@gmail.com>
@ariebovenberg
Copy link
Contributor Author

This has been fixed in #3275, closed.

cofin pushed a commit that referenced this issue Apr 1, 2024
@peterschutt @ariebovenberg @cofin
ci: run slotscheck cli as ci job (#3275)
25dc4f5 
This PR moves running slotscheck from within pre-commit to its own job in ci (same as we've done with other tools that require the configured environment to run).

This partially fixes an issue identified in #3251 where the `litestar` package was inadvertently excluded from this check due to an incorrect regex pattern.

Applies fixes for issues identified by the tool.

Co-authored-by: Arie Bovenberg <a.c.bovenberg@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug 🐛 This is something that is not working as expected
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix: address __slots__ and __all__ misconfigurations ariebovenberg/litestar
2 participants
@ariebovenberg @peterschutt