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

Additional allowed magic attributes; consider 'blocklist' over 'allowlist' approach (WPS609) #2268

Closed
ariebovenberg opened this issue Nov 27, 2021 · 2 comments · Fixed by #2281

Comments

@ariebovenberg
Copy link
Contributor

ariebovenberg commented Nov 27, 2021

I'd like to suggest some exceptions to rule WPS609 (disallow accessing magic attributes/methods):

  • __bases__ (the base classes of a class)
  • __path__ (present in Python packages)
  • __dict__ (present in all non-slots classes)
  • __module__ (present on all Python classes, functions)

These methods are part of established Python standards and are explicitly documented as being readable (and sometimes even settable!).

I could add these to _allowed_magic_attributes, but... I'd like to go one step further and suggest only generating errors for magic methods which have a possible alternative. For a few reasons:

  1. Currently we force people to noqa without offering a workaround (e.g. how do you get a class bases without using __bases__?)
  2. Some modules define their own (documented) magic attributes (like enum defining __members__) . This style guide cannot predict what the allowed/disallowed behavior is for all other libraries (current and future).
  3. By keeping a 'blocklist' we still prevent all known egregious violations, like calling __eq__, __iter__, __delitem__, etc. directly. We can add more as we encounter them.

Also: If forbidding introspection is what we're after, it would perhaps be better to create a separate violation (so that it can be toggled more easily for introspection-heavy projects)

related:

@ariebovenberg
Copy link
Contributor Author

ariebovenberg commented Nov 28, 2021

@sobolevn I'm happy you gave your blessing to add extra allowed attributes (dry-python/returns#1147 (review)) 🙏 . How do you feel about maintaining a list of 'forbidden attributes' instead of a list of 'allowed' ones? Valid ones are easily missed (and more could be added in modules/libraries without WPS knowledge), while we can find all 'forbidden' ones here.

@sobolevn
Copy link
Member

forbidden attributes

Yes, this actually looks like a great idea! Because there might be extra magic attributes that are exposed as 1-party API members. This seems like being too strict for no valid reason.

We can safe forbig all magic methods that have other ways to be called. Possibly, some extra ones.

Thanks a lot for another great feature request! 👍

@ariebovenberg ariebovenberg changed the title Additional allowed magic attributes; consider 'blocklist' over 'allowlist' approach (WPS603) Additional allowed magic attributes; consider 'blocklist' over 'allowlist' approach (WPS609) Nov 28, 2021
ariebovenberg added a commit to ariebovenberg/wemake-python-styleguide that referenced this issue Dec 2, 2021
sobolevn added a commit that referenced this issue Dec 2, 2021
* only disallow certain magic attributes (#2268)

* use already defined magic methods in constants

* Update wemake_python_styleguide/constants.py

* add changelog entry about fixing some forgotten magic methods

Co-authored-by: Nikita Sobolev <mail@sobolevn.me>
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 a pull request may close this issue.

2 participants