False positive for WPS125 and attributes

[sobolevn]

Bug report

What's wrong

  76:1     WPS125 Found builtin shadowing: __doc__
  _coalesce_result.__doc__ = __doc__
  ^

It is not technically builtin shadowing.

[Dreamsorcerer]

Also:

class Foo:
    id = 5

[skarzi]

Example provided by @Dreamsorcerer is really common especially when working with ORMs, it's quite common to name fields/descriptors id or type, so it would be also great enhancement 👍

[skarzi]

Even following one:

def foo(bar):
    bar.id = 12345

[orsinium]

This is the rule that is hard to have done well, IMHO.

1. It's ok to shadow some built-ins. Namely: credits, copyright, license.
2. It's ok to shadow any built-in on the class attributes level because of ORM and data classes.
3. And it's not ok at the same time:

   class A:
       min = Field(...)
       max = Field(default=min(1,2,3))

   it raises something like 'field' object is not callable

Python 🤷

[Dreamsorcerer]

I think 3 is an edge case that's better handled by type checking (e.g. Mypy), rather than worrying about it in wemake.
The error can only occur within the definition of the class attributes, so it's not something that's likely to happen very often.

[Dreamsorcerer]

Alternatively, a single # noqa when defining the attribute would be acceptable. As long as using the attribute later on doesn't cause a violation as in the other examples (e.g. A.min = 5).

[Dreamsorcerer]

OK, thinking about it, I think the latter is the better answer, have opened a PR to add this information to the docs.

[sobolevn]

Ok, we have lots of bugs with attributes here.

First of all, I want to change how some rules are applied for not self/cls/mcs targets.
For example, point.x = 1 should not be a length violation, because point might have this specific API.
Also, we don't want to raise violations on things like @Dreamsorcerer mentioned in #1327 (comment)

I also want to ignore license and some other names mentioned by @orsinium in #1327 (comment)

So, I will refector a lot of stuff to get this correct. Future reviews are welcome!

[sobolevn]

So, basically we would have two sets of rules for attributes:

• For all attributes, for example [any].__prop = 1 is a violation (a couple of rules fit into this category)
• For self*-ish attributes, where self.x = 1 is a violation (almost all our rules will be in this category)

[sobolevn]

I am not going to create a new issue for it, because I am working on it right now anyways, but this is also a bug:

class Some(object):
    UPPER, lower = 1, 2

It does not raise any violations, but it should raise:

  2:5      WPS115 Found upper-case constant in a class: UPPER
  UPPER = 1