Fix for issue 649. #678

NanduTej · 2019-07-18T10:37:31Z

I have made things!

Checklist

I have double checked that there are no unrelated changes in this pull request (old patches, accidental config files, etc)
I have created at least one test case for the changes I have made
I have updated the documentation for the changes I have made
I have added my changes to the CHANGELOG.md

Related issues

NanduTej · 2019-07-18T10:39:01Z

Hey @sobolevn! I have made the changes and I think they work as they are supposed to. Can you check and let me know? This is code for #649

sobolevn · 2019-07-18T10:41:38Z

Looking good!
Can you please rebase your changes? And I will merge them into 0.12

NanduTej · 2019-07-18T10:53:47Z

@sobolevn I did the rebase.

wemake_python_styleguide/visitors/ast/keywords.py

sobolevn · 2019-07-18T11:23:03Z

@NanduTej please, fix style 🙂

You can run make test locally to get the same checks CI has.

wemake_python_styleguide/visitors/ast/keywords.py

NanduTej · 2019-07-18T12:24:17Z

@sobolevn I think my parts of the code got through. There are errors in the previous code.

sobolevn · 2019-07-18T12:40:10Z

There are no issues visible to CI in the projects right now: https://travis-ci.org/wemake-services/wemake-python-styleguide/builds

But, new rules - reveal new problems! That's why we build new rules 💪
There are several things to fix here in your PR.

First issue

    def _check_first_element(
        self,
        node: ast.AST,
        statement: ast.AST,
        extra_lines: int,
    ) -> Optional[bool]:
        if statement.lineno == node.lineno and not extra_lines:
            return False
        return None

This code should not raise any violations. It is correct. See the original issue (3rd example):

This code from ./wemake_python_styleguide/logic/naming/name_nodes.py is also correct:

def get_assigned_name(node: ast.AST) -> Optional[str]:
    """
    Returns variable names for node that is just assigned.

    Returns ``None`` for nodes that are used in a different manner.
    """
    if isinstance(node, ast.Name) and isinstance(node.ctx, ast.Store):
        return node.id

    if isinstance(node, ast.Attribute) and isinstance(node.ctx, ast.Store):
        return node.attr

    if isinstance(node, ast.ExceptHandler):
        return getattr(node, 'name', None)

    return None  # this is a meaningful value!

Please, make sure that all the examples from the original task work.
Add them in tests if they are missing right now.

Second issue

./wemake_python_styleguide/visitors/ast/keywords.py
  215:1    WPS214 Found too many methods: 8
  @final
  ^

It means that your new method makes the class too complex. Here's how to fix it:
Move some logic from the class. How to define that this is pure logic and not methods? Look at _get_return_node_variables and _get_name_nodes_variable and _get_assign_node_variables - they don't use self at all (we might actually build a new rule to check that). It means that they can be extracted to logic/returns.py file. Then you will have extra capacity to add your new method without this violation.

Thanks you for your work! I really appreciate it 👍
Let's make an awesome product together! 🎉

Please enter the commit message for your changes. Lines starting

NanduTej · 2019-07-19T07:46:59Z

@sobolevn sorry for spamming so many commits, my flake8 does not give me the full messages as the build was giving me, so I pushed all the changes I could see and tested them. I am still confused as to what the issue is with the 2 'return None'. I have added all the tests mentioned in the issue and all of them are passing. Can you help me out?

sobolevn · 2019-07-19T07:49:56Z

@NanduTej the easiest way to fix it is:

create ex.py with the code that raises a violation
run it with scripts/parse.py to see the ast, it might give you a hint on what is going on
debug your visitor on why it is happening: use print(node, vars(node)) or pdb
add this code sample to the tests, so we can be sure that the problem is fixed, more tests = better product

sobolevn · 2019-07-19T07:58:40Z

If you setup does not match the one from CI, try the clean build:

deactivate
rm -rf dist pip-wheel-metadata wemake_python_styleguide.egg-info .venv
poetry install
poetry run make test

NanduTej · 2019-07-19T10:17:29Z

If you setup does not match the one from CI, try the clean build:
deactivate
rm -rf dist pip-wheel-metadata wemake_python_styleguide.egg-info .venv
poetry install
poetry run make test

Thanks! This worked.

sobolevn · 2019-07-21T10:23:08Z

@NanduTej how's it going? Do you need any help from my side? 🙂

NanduTej · 2019-07-21T15:31:32Z

@sobolevn Sorry, didn't finish the work on this. I am out for my convocation ceremony😄. Will get back and work on it in 3 days.

sobolevn · 2019-07-21T16:45:00Z

Had to google what "convocation ceremony" is 😆
Congratulations! 🎉

Feel free to get back to it when you have the time.

NanduTej · 2019-07-25T11:05:11Z

Hey @sobolevn! I tried to debug but could not get anywhere. Can you help me out further? I did create the ex.py file and pasted the 2 pieces of code and ran the parse.py program. I did not find anything out of order in the output.

sobolevn · 2019-07-25T11:37:57Z

@NanduTej what exactly is your problem?

NanduTej · 2019-07-25T11:41:08Z

On running flake8 ., There is an error popping up.

./wemake_python_styleguide/logic/naming/name_nodes.py

29:5 WPS331 Found local variable that are only used in return statements
return None
^

./wemake_python_styleguide/visitors/ast/statements.py

219:9 WPS331 Found local variable that are only used in return statements
return None
^

I am unable to debug this.

sobolevn · 2019-07-25T11:45:21Z

wemake_python_styleguide/visitors/ast/keywords.py

-        returns, return_sub_nodes = self._get_return_node_variables(nodes)
+        assign = get_assign_node_variables(nodes)
+        names = get_name_nodes_variable(nodes)
+        returns, return_sub_nodes = get_return_node_variables(nodes)


You can insert breakpoint() (if you are using 3.7, otherwise see https://realpython.com/python-debugging-pdb/) and debug this function.

You can also use ipbd:

pip install ipdb

export PYTHONBREAKPOINT=ipdb.set_trace

Set breakpoint() where you need it

Run flake8 on file that raises an unwanted violation

NanduTej · 2019-07-26T07:49:39Z

Hey @sobolevn! Can you explain this to me?

wemake-python-styleguide/tests/test_checker/test_noqa.py

Line 87 in b6b2738

'WPS331': 1,

sobolevn · 2019-07-26T08:13:55Z

@NanduTej this is a integration test for several things:

that violation is active and enabled
that violation is raised for the bad code
that line number where violation is raised is correct
that noqa works
Docs: https://github.com/wemake-services/wemake-python-styleguide/blob/master/CONTRIBUTING.md#before-submitting

NanduTej · 2019-07-31T07:22:10Z

Hey @sobolevn! I am sorry I am taking so much time with this. Can you help me with understanding the flow of how flake8 works. Maybe that will give me a clue as to where to look for the error.

sobolevn · 2019-07-31T07:30:54Z

@NanduTej can you please show me the piece of code that does not work for you? Maybe I can help you from there. Since, I am pretty sure that flake8 has nothing to do with this task.

(In case you still need this information, it is right in our docs: https://wemake-python-stylegui.de/en/latest/pages/api/index.html#overview and here's how all classes work: https://wemake-python-stylegui.de/en/latest/pages/api/index.html#api-reference)

NanduTej · 2019-07-31T07:33:15Z

On running flake 8, I run into this error.

./wemake_python_styleguide/logic/naming/name_nodes.py
29:5 WPS331 Found local variable that are only used in return statements
return None
^

./wemake_python_styleguide/visitors/ast/statements.py

219:9 WPS331 Found local variable that are only used in return statements
return None
^

The code is executing fine with all my tests. When I push my code, this error will make build fail which is coming when flake8 command is being run.

sobolevn · 2019-07-31T08:04:49Z

So, then add these cases to your tests:

correct_example_N = """
def get_assigned_name(node: ast.AST) -> Optional[str]:
    if isinstance(node, ast.Name) and isinstance(node.ctx, ast.Store):
        return node.id

    if isinstance(node, ast.Attribute) and isinstance(node.ctx, ast.Store):
        return node.attr

    if isinstance(node, ast.ExceptHandler):
        return getattr(node, 'name', None)

    return None
"""

And:

correct_example_N1 = """
class WrongAttributeVisitor(object):
     def _is_super_called(self, node: ast.Call) -> bool:
        if isinstance(node.func, ast.Name):
            if node.func.id == 'super':
                return True
        return False
"""

And then debug what is going on: I wrote a debugging section of documentation just for this case.
https://wemake-python-stylegui.de/en/latest/pages/api/debugging.html

P.S. Don't forget to rebase your code.

NanduTej · 2019-07-31T08:50:50Z

wemake_python_styleguide/logic/returns.py

+    return_sub_nodes: Dict[str, ast.Return] = defaultdict()
+    for sub_node in node:
+        if isinstance(sub_node, ast.Return):
+            if isinstance(sub_node.value, ast.Name):


For the correct_example_N we are not collecting the previous returns where I expected them to be collected in return_sub_nodes and so we are running into issues here. The correct_example_N1 is passing the tests. How should we collect those returns here? Example the returns of type ast.Attribute, and should we be collecting them here?
@sobolevn

Sorry, I have missed your question. Can you try to use ast.walk(subnode) there? I guess that happens because they are not direct children of the give node, but are lower down the tree.

You can try some other task if you are stuck with one. And pass this one to me.

sobolevn approved these changes Jul 18, 2019

View reviewed changes

NanduTej force-pushed the issue-649 branch from 6667ce4 to 5fbfff2 Compare July 18, 2019 10:52

sobolevn requested changes Jul 18, 2019

View reviewed changes

wemake_python_styleguide/visitors/ast/keywords.py Outdated Show resolved Hide resolved

sobolevn requested changes Jul 18, 2019

View reviewed changes

wemake_python_styleguide/visitors/ast/keywords.py Outdated Show resolved Hide resolved

NanduTej force-pushed the issue-649 branch from 836c0c8 to b2a401d Compare July 19, 2019 06:12

NanduTej added 7 commits July 19, 2019 12:43

Fix for issue 649.

be0f57e

Adding type to function added

c29d8ce

Making style changes

e3e8396

Style fixes-2.

8efe27d

Style fixes-3.

3679440

Please enter the commit message for your changes. Lines starting

Style fixes-4.

c65d4c7

Style fixes-5.

d805232

NanduTej force-pushed the issue-649 branch from 89f5fb2 to d805232 Compare July 19, 2019 07:16

style fixes-6.

7d5f540

NanduTej force-pushed the issue-649 branch from 7be886d to 7d5f540 Compare July 19, 2019 07:36

sobolevn requested changes Jul 25, 2019

View reviewed changes

NanduTej commented Jul 31, 2019

View reviewed changes

sobolevn closed this in c47b828 Aug 10, 2019

ragohin mentioned this pull request Dec 9, 2019

Warn about possibly infinite while #1007

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for issue 649. #678

Fix for issue 649. #678

NanduTej commented Jul 18, 2019

NanduTej commented Jul 18, 2019 •

edited

Loading

sobolevn commented Jul 18, 2019 •

edited

Loading

NanduTej commented Jul 18, 2019

sobolevn commented Jul 18, 2019

NanduTej commented Jul 18, 2019 •

edited

Loading

sobolevn commented Jul 18, 2019 •

edited

Loading

NanduTej commented Jul 19, 2019 •

edited

Loading

sobolevn commented Jul 19, 2019

sobolevn commented Jul 19, 2019

NanduTej commented Jul 19, 2019

sobolevn commented Jul 21, 2019

NanduTej commented Jul 21, 2019

sobolevn commented Jul 21, 2019

NanduTej commented Jul 25, 2019

sobolevn commented Jul 25, 2019

NanduTej commented Jul 25, 2019

sobolevn Jul 25, 2019

sobolevn Jul 25, 2019

NanduTej commented Jul 26, 2019

sobolevn commented Jul 26, 2019 •

edited

Loading

NanduTej commented Jul 31, 2019

sobolevn commented Jul 31, 2019

NanduTej commented Jul 31, 2019 •

edited

Loading

sobolevn commented Jul 31, 2019

NanduTej Jul 31, 2019

sobolevn Aug 4, 2019

sobolevn Aug 4, 2019

NanduTej Aug 10, 2019

Fix for issue 649. #678

Fix for issue 649. #678

Conversation

NanduTej commented Jul 18, 2019

I have made things!

Checklist

Related issues

NanduTej commented Jul 18, 2019 • edited Loading

sobolevn commented Jul 18, 2019 • edited Loading

NanduTej commented Jul 18, 2019

sobolevn commented Jul 18, 2019

NanduTej commented Jul 18, 2019 • edited Loading

sobolevn commented Jul 18, 2019 • edited Loading

First issue

Second issue

NanduTej commented Jul 19, 2019 • edited Loading

sobolevn commented Jul 19, 2019

sobolevn commented Jul 19, 2019

NanduTej commented Jul 19, 2019

sobolevn commented Jul 21, 2019

NanduTej commented Jul 21, 2019

sobolevn commented Jul 21, 2019

NanduTej commented Jul 25, 2019

sobolevn commented Jul 25, 2019

NanduTej commented Jul 25, 2019

sobolevn Jul 25, 2019

Choose a reason for hiding this comment

sobolevn Jul 25, 2019

Choose a reason for hiding this comment

NanduTej commented Jul 26, 2019

sobolevn commented Jul 26, 2019 • edited Loading

NanduTej commented Jul 31, 2019

sobolevn commented Jul 31, 2019

NanduTej commented Jul 31, 2019 • edited Loading

sobolevn commented Jul 31, 2019

NanduTej Jul 31, 2019

Choose a reason for hiding this comment

sobolevn Aug 4, 2019

Choose a reason for hiding this comment

sobolevn Aug 4, 2019

Choose a reason for hiding this comment

NanduTej Aug 10, 2019

Choose a reason for hiding this comment

NanduTej commented Jul 18, 2019 •

edited

Loading

sobolevn commented Jul 18, 2019 •

edited

Loading

NanduTej commented Jul 18, 2019 •

edited

Loading

sobolevn commented Jul 18, 2019 •

edited

Loading

NanduTej commented Jul 19, 2019 •

edited

Loading

sobolevn commented Jul 26, 2019 •

edited

Loading

NanduTej commented Jul 31, 2019 •

edited

Loading