0509 fails at Comments section

[szabolcstoth]

Describe the bug
I started to play with issue #174 and I ran into an issue.
If data is defined before the first data section, that part counts as Comments section (lines are considered as COMMENT tokens).
Currently, the mentioned 0509 Section is empty rule will fail for Comments section unless there is only one line which starts with robocop: string due to the check_if_empty function.

for child in node.body:
    if isinstance(child, Comment):
        if 'robocop:' in child.tokens[0].value and len(node.body) == 1:
            break
        elif not isinstance(child, EmptyLine):
            break
    else:
        self.report("empty-section", node=node)

To Reproduce
You can reproduce the issue with this test code:

Data before the first section is ignored in Robot Framework.
Second line in Comment section.

*** Settings ***
Documentation    Dummy documentation.


*** Test Cases ***
Dummy Test Case
    [Documentation]    Dummy documentation.
    No Operation

This one will pass.

robocop:Data before the first section is ignored in Robot Framework.
*** Settings ***
Documentation    Dummy documentation.


*** Test Cases ***
Dummy Test Case
    [Documentation]    Dummy documentation.
    No Operation

Expected behavior
I suppose we should handle Comments section like the others.

Additional context
Is there any particular reason why the check_if_empty function was implemented this way?
If this is a bug, it should be an easy fix by removing the if 'robocop: ... part, I can do a pull request for it.

[bhirsz]

I don't know why there is len(body) == 1 - clearly a mistake.

The robocop: part is needed because this is syntax used to disable rule warning from particular line ie:

This break not capitalized keyword Name  # robocop: disable=not-capitalized-keyword-name

or simply

This break not capitalized keyword Name  # robocop: disable

We can do block disablers so this is possible:

*** Settings ***
# robocop: disable
Stuff
...
# robocop: enable

The point is that if you have section only with robocop disabler, it's esentially empty (so should trigger warning). The exception is Comments section - because if we want to disable whole file in robocop from source code, robocop: syntax should be placed in very first line (making it hidden *** Comments ***).

How this rule should look like? I think that:

1. If it's *** Comments *** section, just check if there is anything except Empty Lines. This should trigger warning:

*** Comments ***

*** Settings ***

and this not

*** Comments ***
Some general comment about suite


*** Settings ***

2. If it's any other section it should trigger warning if there are only EmptyLines and Comments. So this also should trigger it:

*** Variables ***
# ${var}  10  only one line and commented out for some reason, making section 'empty'

Regarding to connection with "data before first section rule (#174) this is fine:

# robocop: disable=some-specific-rule-that-I-want-to-ignore-in-this-whole-file
*** Test Cases ***
stuff

this is also fine:

*** Comments ***
Someone clearly used Comments section so we don't need to tell him/her that this section is ignored
*** Test Cases ***
stuff

and this is not fine:

${var}  10  # ups, this is comment not effective code!
*** Keywords ***
Keyword

[szabolcstoth]

Thanks for the explanation.

I agree with your idea, in this case I suppose we can fix/improve 0509 by changing check_if_empty function like this:

    def check_if_empty(self, node):
        anything_but = EmptyLine if isinstance(node, CommentSection) else (Comment, EmptyLine)
        for child in node.body:
            if not isinstance(child, anything_but):
                break
        else:
            self.report("empty-section", node=node)

For this, we need to import at the beginning:

from robot.parsing.model.blocks import CommentSection

This change can cover what you wrote:

• If it's *** Comments *** section, just check if there is anything except Empty Lines.
• If it's any other section it should trigger warning if there are only EmptyLines and Comments.

Should I create a PR?

[bhirsz]

Looks very good! You can go ahead with PR.

Just simple suggestion:

def check_if_empty(self, node):
        anything_but = EmptyLine if isinstance(node, CommentSection) else (Comment, EmptyLine)
       if all(isinstance(child, anything_but) for child in node.body):
            self.report("empty-section", node=node)