robotframework-robocop icon indicating copy to clipboard operation
robotframework-robocop copied to clipboard

[Bug] Rule #1005 ignores comment lines

Open antonpaa opened this issue 2 years ago • 8 comments

What happened?

An example code

Keyword1
     Keyword2

# Comment

Keyword3
     Keyword4

won't pass the rule 1005 (empty-lines-between-keywords). The received output will be as follows:

Invalid number of empty lines between keywords (2/1)

It's down to preferences, but as a RF user I see use cases for having e.g. keyword library section separators (and multiple other use cases) that could be done with single-line comments between keywords.

Operating System

Windows

Robocop version

2.0.2

antonpaa avatar May 16 '22 06:05 antonpaa

It could be parametrized - ie "ignore_comments" parameter. Or we could ignore comments with specific patterns (such as starting with triple ###).

bhirsz avatar May 16 '22 06:05 bhirsz

Parameterizing the option to ignore comments could make sense to me, as this syntax could be a yellow flag for some other projects. Introducing a specific pattern (e.g. ###) is perhaps a bit too much pushing towards a non-existing style guide.

antonpaa avatar May 16 '22 06:05 antonpaa

I've started working on this (I started from adding the parameter etc) but I quickly got stuck. I need we need to rethink how the rule is checking the empty lines by default. In your example:

image

The comments are ignored, so it yields 2 empty lines - I think it's totally unexpected behaviour. Also it does not differentiate between comments that stay have the same indentation as keywords and "standalone" comments:

image

(3/1 empty lines detected).

As bare minimum it should:

  • differentiate between comments that still belongs to the keyword, and those that don't (so 3/1 situation would become 2/1)
  • then it should only count empty lines between keyword and first 'standalone' comment (comment starting from the beginning of the line) - 2/1 case would become 1/1

Then we can add extra parameter that would check if empty lines between/after standalone comments are okay too (check_standalone_comments or smth). Example: image

Yellow ones should be checked by default, blue ones when using extra parameter/flag.

Thoughs @mnojek ?

bhirsz avatar Jul 29 '22 06:07 bhirsz

We could by default check after/before keyword empty spaces with standalone comments, like here: image

(every yellow space should contain only 1/1 empty lines)

but there is case where people are using comments right before keywords, and the default rule would then trip: image

bhirsz avatar Jul 29 '22 06:07 bhirsz

Personally I think that this approach is near a somewhat expected behavior.

We could by default check after/before keyword empty spaces with standalone comments, like here: image

(every yellow space should contain only 1/1 empty lines)

but there is case where people are using comments right before keywords, and the default rule would then trip: image

If the rule allows max 1 empty line before & after for a comment - or in other words is capable of ignoring a surrounding empty line if such exists - then I think it would be both flexible but would still catch unintended double empty lines, whether with comments or not. The complexity kicks in in cases like

Keyword
    Step

# comment

Keyword2
...

as for a parser ignoring empty lines around comments it shouldn't either look like this:

Keyword
    Step
Keyword2
...

Do you think such a functionality is achievable?

antonpaa avatar Jul 29 '22 08:07 antonpaa

I would say that the rule should not be too complicated and it should not allow more than 1 line between comments. If we ignore comments, users shouldn't be forced to understand what special conditions they need to follow. It should be intuitive. Ignoring comments should ignore all comments and lines around them, but the rule should be violated when there are 2 or more consecutive empty lines.

Example:

This should fail (note 2 consecutive empty lines between comment 3 and comment 4):

Keyword 1
    Step

# comment 1

    # comment 2
    # comment 3


# comment 4

Keyword 2
    Step

but this should be fine:

Keyword 1
    Step

# comment 1

    # comment 2
    # comment 3

# comment 4

Keyword 2
    Step

WDYT? @bhirsz @antonpaa

mnojek avatar Jul 29 '22 10:07 mnojek

Yes, fully agreed that this would be an ideal definition!

antonpaa avatar Jul 29 '22 11:07 antonpaa

@mnojek I agree, this will be intuitive for the users

bhirsz avatar Jul 29 '22 11:07 bhirsz

@antonpaa Issue was fixed and released in Robocop 2.5.0 yesterday - please check and provide the feedback if necessary :)

bhirsz avatar Sep 15 '22 08:09 bhirsz