Wemake-python-styleguide: Forbid `Union[T, None]`, use `Optional[T]` instead

[sobolevn]

Rule request

Thesis

One can write Optional[T] to indicate that T and None can be returned.
This is a valid way to do it.

But Union[T, None] is invalid. And should be forbidden.

I have to note that Union[A, B, C, None] is ok.

Reasoning

This is a consistency rule.

[Soniyanayak51]

Hi @sobolevn , I would like to work on this. Do I need to add checks wherever I find anUnion[ ]?

[sobolevn]

@Soniyanayak51 awesome! Yes, you need to check that Union[ ... ] subscript does not contain None.

Assigned 👍

[Soniyanayak51]

@sobolevn should I just raise an error with a message?

[sobolevn]

@Soniyanayak51 you should create a new violation here: https://github.com/wemake-services/wemake-python-styleguide/blob/master/wemake_python_styleguide/violations/consistency.py

And raise it like so: https://github.com/wemake-services/wemake-python-styleguide/blob/master/wemake_python_styleguide/visitors/ast/builtins.py#L169

I think that wemake_python_styleguide.visitors.ast.builtins:WrongSubscriptVisitor (new class) is the best place for it. Don't forget to add this new class to the preset: https://github.com/wemake-services/wemake-python-styleguide/blob/master/wemake_python_styleguide/presets/types/tree.py

[Soniyanayak51]

@sobolevn one more question, in wemake-python-styleguide/wemake_python_styleguide/violations/consistency.py, should be a ASTViolation or TokenizeViolation? Sorry I am not familiar with the codebase.

[sobolevn]

This should help you: https://wemake-python-stylegui.de/en/latest/pages/api/tutorial.html#writing-new-visitor

TLDR: ASTViolation

[Soniyanayak51]

@sobolevn
in the line you referenced,

def _check_unpacking_targets(
        self,
        node: ast.AST,
        targets: Iterable[ast.AST],
    ) -> None:
        for target in targets:
            target_name = extract_name(target)
            if target_name is None:  # it means, that non name node was used
                self.add_violation(WrongUnpackingViolation(node))

I am having trouble in adding Union to the argument list.

I was thinking of using __args__ attribute but I don't know how to use ast.

[sobolevn]

We have severals guides on ast just inside our docs: https://wemake-python-stylegui.de/en/latest/pages/api/contributing.html
We also have helper tools to visualise the tree you are working with: https://wemake-python-stylegui.de/en/latest/pages/api/contributing.html#helpers
And here's our debugging guide: https://wemake-python-stylegui.de/en/latest/pages/api/debugging.html

[Soniyanayak51]

@sobolevn I added the violation NoneInSubscriptViolation. This is the class I want to add to builtins.py . Can you please tell how to use Union as targets? pytest fails because I am using ast.AST wrongly as union argument.

class WrongSubscriptVisitor(base.BaseNodeVisitor):
    """Forbids None in Union[..] subscript."""

    def _check_none_targets(
        self,
        node: ast.AST,
        targets: Union[ast.AST].__args__,
    ) -> None:
        if len(targets) == 2:
            for target in targets:
                if target is type(None):  # it means, that None was used in arguments
                    self.add_violation(consistency.NoneInSubscriptViolation(node))

[sobolevn]

Sorry, I don't understand your question.

[Soniyanayak51]

I mean how can i iterate over the Union[..] subscript. I use targets as the Union arguments. How do i declare the arguments of the _check_none_targets function.

[sobolevn]

Please, have a look at the docs. We have three ways of looking at the internal structure of an object:

1. Debugging, the link is available in the comment above
2. Documentation about the ast, it is available in the contributing docs
3. Helper scripts inside the ./scripts folder. The docs are also available

[orsinium]

@Soniyanayak51 nice to see you take care of it! I think, you can see at merged PRs for good examples how to do such stuff. For example, #836 looks good. Try to see examples, documentation, it will help you a lot.

If this is your first-time contribution, I recommend starting from something less complex. For example, https://github.com/life4/flakehell/issues/17 related to this project, has example of implementation and much less complex :+1:

[sobolevn]

@novikovfred do you want to take this up?

[novikovfred]

@novikovfred do you want to take this up?

Yep, I would like to do it.

[novikovfred]

I'm working on this problem, I need some more time to solve it.

[Soniyanayak51]

@sobolevn Extremely sorry I couldn't complete this.

[sobolevn]

@Soniyanayak51 no problems! 👍 Glad that you have tried to! That's a big success on its own.
Come back anytime you want.

[sobolevn]

Hi @novikovfred, how's it going? Do you need any help?