Rule: Check if there is data before first section

[mnojek]

Any data in a file that is located before the first section is ignored in RF. I think it may be useful to catch such cases and report that it is not contained by any section thus ignored by RF.

[szabolcstoth]

Your comments in #234:

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

Based on these examples I suppose it should be enough to check whether the first line of the file is a data section header or not (using RawFileChecker).

It can be done like:

def visit_File(self, node):
    tokens = list(get_tokens(node.source))
    if not 'HEADER' in tokens[0].type and not tokens[0].value.startswith('# robocop:'):
        self.report(...)

Or like this:

def parse_file(self):
    self.lines = []
    super().parse_file()
    if not self.lines[0].startswith('***') and not self.lines[0].startswith('# robocop:'):
        self.report(...)

Should we put this to the comments.py checker category?

[bhirsz]

The second option will be probably faster. But what if someone have only empty lines before first section? :

*** Settings ***

should we report 'data before first section is ignored' or rather something along of 'invalid number of empty lines before section 4/0'?

[szabolcstoth]

Good question, it depends on the point of view.

From the logical perspective (and I suppose from Robot Framework's point of view), it is considered as data, even if they are empty lines. And if you merge my pull request about 0509, Robocop will throw a warning with [W] 0509 Section is empty, since those empty lines before the *** Settings *** are considered as part of the *** Comments ***.

I would throw a warning with data before first section is ignored instead of invalid number of empty lines before section.
What is your opinion?

[bhirsz]

Yes I agree with you 👍

[szabolcstoth]

Thanks! :)

Since this data is considered as Comments, should I put the new rule inside comments.py? Or is length.py even better?
Something like:

class IgnoredDataChecker(RawFileChecker):
    """ Checker for ignored data. It checks whether there is ignored data defined before the first section header or not."""
    rules = {
        "0527": (
            "ignored-data",
            "Found data in file which is ignored",
            RuleSeverity.WARNING
        )
    }

[bhirsz]

It's not really checking for length, so comments.py would be more suitable. Although I would argue about half of the rules grouping in robocop - it's not important to end user and it's only for splitting up a code a bit and helping to find given rule faster. And if I were to look for "comment section related rules" I would hit comments.py first.

[szabolcstoth]

There is another situation to handle (beside the three example above), it is not enough to check the first line, as I assumed earlier:

# robocop: disable=1001
${var}    10    # This should throw a warning.
${var}    20    # This is not good but one warning is enough.
*** Settings ***

Based on all the scenarios, what about this?

class IgnoredDataChecker(RawFileChecker):
    """ Checker for ignored data. """
    rules = {
        "7004": (
            "ignored-data",
            "Ignored data found in file",
            RuleSeverity.WARNING
        )
    }

    def __init__(self, *args):
        self.section_header = False
        self.ignored_data = False
        super().__init__(*args)

    def check_line(self, line, lineno):
        if self.section_header or self.ignored_data:
            return
        if line.startswith('***'):
            self.section_header = True
            return
        if not line.startswith('***') and not line.startswith('# robocop:'):
            self.report("ignored-data", lineno=lineno, col=0)
            self.ignored_data = True

I can also do the checks in the def parse_file(...), but it would mean we will go through the first lines twice (once while reading the lines, once during the for loop).

    def parse_file(self):
        self.lines = []
        super().parse_file()
        for line in self.lines:
            if line.startswith('***'):
                break
            if not line.startswith('***') and not line.startswith('# robocop:'):
                self.report("ignored-data", lineno=1, col=0)
                break

    def check_line(self, line, lineno):
        self.lines.append(line)

[bhirsz]

If you go with second approach you don't need to call suepr().parse_file() if you're iterating over lines anyway. You can do something like:

    def parse_file(self):
        """ Read file line by line and for each call check_line method. """
        with open(self.source) as file:
            for lineno, line in enumerate(file, 1):
                # run your check here, break early if needed

You will iterate over same line only once that way.
From robocop point of view, it just call scan_file() and later accumulate found issues from self.issues, so we don't need to keep same logic in every rule