wemake-services / wemake-python-styleguide Goto Github PK

Names like __some_name should be forbiden, since there's no single use-case in python for real private variables.

Use public and protected variables instead.

When I do pipenv install -d on MacOS, the pluggy=0.6.0 dependency isn't getting installed because of the wrong hash: THESE PACKAGES DO NOT MATCH THE HASHES FROM Pipfile.lock!.

The package version pipenv tries to install is: https://files.pythonhosted.org/packages/ba/65/ded3bc40bbf8d887f262f150fbe1ae6637765b5c9534bd55690ed2c0b0f7/pluggy-0.6.0-py3-none-any.whl

The problem can be caused by pipenv itself (see pypa/pipenv#210).

Since I'm not sure I'm asking to verify that on other platforms.

Sometime I can see code like this:

try:
    import django  # noqa
except ImportError:
    pass

This bare noqa comment covers all possible issues on this line.
That's not what we want. We need to specify exact error numbers:

try:
    import django  # noqa: F401, Z101
except ImportError:
    pass

Models

• no signal, because they are evil side effects
• no BooleanField with null, use NullBooleanField
• no CharField with null, use blank=True with possible '' as default
• use CIEmailField instead of EmailField
• no JSONField other than django.contrib.postgres.fields.JSONField
• no default={} inJSONField, use default=dict instead
• no methods on models except magic ones

Structure

• all applications should be named as *_app
• apps.py and models.py must be present
• check filenames in *_app folder, whitelist: models, views, forms, admin, serializers, apps, urls, tasks, __init__

I fell in love with poetry. It works like a charm for python libraries.
Here's an example: https://github.com/mimesis-lab/mimesis-factory

Currently our linter is full of magic constants like:

• https://github.com/wemake-services/wemake-python-styleguide/blob/master/wemake_python_styleguide/visitors/high_complexity.py#L44
• https://github.com/wemake-services/wemake-python-styleguide/blob/master/wemake_python_styleguide/visitors/high_complexity.py#L58
• and others marked with TODO: config in the source code

What we need is:
0. create a file with default configuration values

1. allow users to override these values with setup.cfg or any other standard flake8 tools
2. test that when configuration is applied linter respects it

I am not sure that this is checked right now.

This is just a string in python3, no need to use it.

Resources:

• https://docs.quantifiedcode.com/python-anti-patterns
• https://github.com/devmanorg/fiasko_bro
• https://github.com/pyta-uoft/pyta

Code samples:

• https://stackoverflow.com/questions/43166571/getting-all-the-nodes-from-python-ast-that-correspond-to-a-particular-variable-w

Styleguides:

• http://gemcheck.evilmartians.io/
• https://google.github.io/styleguide/pyguide.html

We need to be sure that it is possible to suppress our warnings with # noqa comments and other standard flake8 ways to do it.

Reference: http://flake8.pycqa.org/en/3.1.1/user/ignoring-errors.html
Examples of how to do it can be found here: https://github.com/PyCQA/flake8-bugbear/blob/master/bugbear.py#L38

In some cases on line

node_parent = getattr(node, 'parent')

of WrongModuleMetadataVisitor we get

AttributeError: 'Assign' object has no attribute 'parent'

The bug can be fixed by the correct usage of getattr:

node_parent = getattr(node, 'parent', None)

The PR with the fix and a regression test will be ready in a day.

I am not sure about these.

Classes

• restrict passing attributes in __init_subclass__: class MyClass(MetaClass, option=1)

Side effect inside __init__.py are nightmares.

We need to restrict any kind of python code inside __init__.py except:

1. comments
2. docstrings

Imports should be forbidden. Each separate module should have its own public API.

It is not 100% ready yet. But is very close to it.
https://github.com/Miserlou/JonesComplexity

Original issue: wemake-services/wemake-django-template#218

For example, the following is the only way in Django to inherit the Meta class:

class Meta(BaseModel.Meta):
    pass

But unfortunately it triggers WPS100 Found wrong keyword "pass".

More generally, the following should be allowed as it's a common thing to do:

class Foo(BarMixin, FooBase):
    pass

Right now the # noqa comment helps to avoid the warning.

I don't see any reasons why we should use them.
Let's use .format() instead.

P.S. For a reason we can not forbid % usage: gforcada/flake8-pep3101#23

We need to be sure that cases like:

class Model(models.Model):
     class Meta: ...  # error here, needs explicit base class

are covered.

Correct way to write this example is:

class Model(models.Model):
     class Meta(object): ... 

This should work for all NESTED_CLASSES_WHITELIST.

As per the docs:

getattr(x, 'foobar') is equivalent to x.foobar

This may lead a reader to believe that getattr(obj, 'attribute') will not throw an exception.

Correct

x.foobar
getattr(x, 'foobar', None)
getattr(x, some_variable, None)

Incorrect

getattr(x, 'foobar')

We need to check function names length.

This should follow the same length rule as variables.

Variable names like async and await will not work in python3.7.
They should be forbidden even in python3.6.

In my opinion staticmethod should not be used.

It is better to use regular methods or regular functions.
There should be nothing in between.

In python2 we had different classes: class Some: and class Some(object): were totally different things.

Currently, we don't have this thing in python3, but since these times I believe that new-styled classes are still better.

1. It has more semantics: this class is related to object
2. It has unified interface: class Base(object) and class Child(Base) vs class Base and class Child(Base)
3. Explicit is better than implicit

So, I believe we should ban classes with old-styled base class.
General rule is: always write super-classes' names

Correct

class Philosopher(object):
    def __init_subclass__(cls, default_name, **kwargs):
        ...

class AustralianPhilosopher(Philosopher, default_name="Bruce"):
    ...

class WithMetaClass(object, metaclass=MyMeta):
   ...

Wrong

class Philosopher: ...
class AustralianPhilosopher(default_name="Bruce"): ...
class WithMetaClass(metaclass=MyMeta): ...

Currently we just have a legacy version.py file with version inside it.
It duplicates the version information from pyproject.toml.

That's how it should be: python-poetry/poetry#273 (comment)

We need to apply the same rules for functions as we apply for variables.

Related #26

Rules to check and implement:

General

• no nested functions
• no nested classes (Meta as whitelist)
• no unused arguments
• no len(x) == 0 in if conditions
• no always True checks
• no raise (without new exception) not within except
• no raise NotImplemented
• no __author__
• no / linebrakes

Complexity

• too many local variables
• too many arguments
• too many branches
• too many returns
• too many expressions
• too many statements
• too deep nesting

Function calls

• no exec, eval, and compile
• no vars(), no dir()
• no print, pprint
• no input
• no globals(), no locals()
• no short function names

Keywords

• no global, no nonlocal, no del, no pass

Imports

• no from some import * aka star-import
• no from .. import some aka dot import
• no nested imports
• no __import__

Classes

• no return/yield in __init__
• no self override
• self is not used
• first method argument must be cls or self if not staticmethod
• too few public method (__init__ + one other)
• too many public methods
• too many instance attributes
• no magic methods: __del__
• no attributes with default None: class Fixture: some_attribute = None

Variables

• no blacklist variable names: data, result
• no short variable names

Files

• no bom
• no long files, max-file-lines = 600
• no logic in __init__.py
• no __{}__ filenames, except __init__ and __main__

mypy

• follow this pattern: x: List[SomeValue] = [], def cast(element: Element = None) -> str:

As far as I can tell, no fixture triggers the TooShortAttributeNameViolation (see https://github.com/wemake-services/wemake-python-styleguide/blob/master/tests/test_checkers/test_wrong_variable.py#L20).

If this is an issue, feel free to assign it to me.

We should use annotations to provide types.
The only type comment that is allowed is: # type: ignore

You should not define your API like that. Use naming conventions: public, _protected.
And you should not use from some import *. Use named imports.

So, we should ban this anti-pattern.

There's already a plugin for it! https://github.com/Korijn/flake8-assignexp/blob/master/flake8_assignexp.py

But, it is not a thing until python3.8.

We need to add handler to our variable blacklist.

http://wemake-python-styleguide.readthedocs.io/en/latest/_pages/errors.html

I don't like the idea that some files are excluded from linting.
But, I believe that this might be useful for several cases.

But, we need to do it inside the configuration file, not in a separate file.

Why?
Because it is hard to tell which files are ignored, and which files are not.
In configuration it is pretty visible from the very first sight.

Related #53

When code has a lot of if, try, for, and other branches it is hard to follow what is going on.

We can restrict the number of logic branches allowed for a single function.
That's how it should work:

• if is counted as +1 for the complexity, else and elif also counts as +1
• for is counted as +1, else is also counted as +1
• while is counted as +1
• try is counted as +1, each except case is counted as +1
• with is counted as +1

We need to figure out what number of branches is the most pleasant to have. We can consult pylint for this.

Currently I am using subprocess to run tests, that's way to expensive.

What I suggest is:

1. we need to rewrite tests to be more specific, covering only one issue at a time, here are the examples https://github.com/PyCQA/flake8-bugbear/blob/master/tests/test_bugbear.py and https://github.com/gforcada/flake8-builtins/blob/master/run_tests.py

2. we still need to run integration tests with subprocess to be sure, that everything still works. But I feel like one test should be enough.

It is used only for some data-science pacakges, which we obviously do not use.

Other usages of this operator different from matrix multiplications should be banned.

They are not a thing in python3 anymore.
But, we should have a whitelist for things like annotations and some new features that might come.

I also feel like we should save some more error code for imports and move other errors up a bit.

Are there any other magic method except __del__ to be restricted?

Related #44

We have to check that code like this:

class Test:
    def method(self):
        with some_manager():
            try:
                if self.call_method():
                    return True
            except:
                  return False

raises TooDeepNestingViolation error.

The optimal nesting level is 4. So, code under try should not have any nesting.

We need to check, that nested functions respect NESTED_FUNCTIONS_WHITELIST values.

It is possible to write something like this:

import os.path

But instead we should write one of the following:

import os
from os import path

You should not use __del__, as it is non-pythonic, more like cpp.

This method is not even called in several situations.
So, let's forbid it.

• comments
• boolean flags https://twitter.com/DavidKPiano/status/972620672743673856
• assert outside of tests

As we have decided to stick to .format(), we are banning f strings: #32

But, @vergeev have also suggested to forbid 'string with %s' % some.
Here's the backstory:

• gforcada/flake8-pep3101#23 has some false positives, that disallow to use regular modulo operator 5 % 2
• we can only forbid using this operator with string literals, not vars as was suggested in #32 (comment)

So, this would be valid:

some_variable % 'value1'
5 % 2

This would be invalid:

'format me %s' % 'value2'

Current noqa.py misses these two rules.

We need to be sure that they are still skippable.

https://travis-ci.org/wemake-services/wemake-python-styleguide/builds/398604326