Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug] Rule #1005 ignores comment lines #611

Closed
antonpaa opened this issue May 16, 2022 · 9 comments · Fixed by #708
Closed

[Bug] Rule #1005 ignores comment lines #611

antonpaa opened this issue May 16, 2022 · 9 comments · Fixed by #708
Assignees
Labels
bug Something isn't working

Comments

@antonpaa
Copy link

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 antonpaa added the bug Something isn't working label May 16, 2022
@bhirsz
Copy link
Member

bhirsz commented May 16, 2022

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

@antonpaa
Copy link
Author

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.

@bhirsz bhirsz self-assigned this Jul 28, 2022
@bhirsz
Copy link
Member

bhirsz commented Jul 29, 2022

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
Copy link
Member

bhirsz commented Jul 29, 2022

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

@antonpaa
Copy link
Author

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?

@mnojek
Copy link
Member

mnojek commented Jul 29, 2022

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

@antonpaa
Copy link
Author

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

@bhirsz
Copy link
Member

bhirsz commented Jul 29, 2022

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

@bhirsz
Copy link
Member

bhirsz commented Sep 15, 2022

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
3 participants