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

Check indentation completely #504

Closed
MoreFamed opened this issue Sep 17, 2021 · 11 comments · Fixed by #731
Closed

Check indentation completely #504

MoreFamed opened this issue Sep 17, 2021 · 11 comments · Fixed by #731

Comments

@MoreFamed
Copy link

I suggest to implement all-catching check of indentation. The current rules regarding indentation ([W1007] uneven-indent and [E1008] bad-indent) allow me to indent very sloppily. 😄 It could work roughly like this:

  • One level of indentation is either one tab or 4 spaces (this "4" would be default, but configurable)
  • Names of keywords and test cases in definition are not indented. Their bodies are indented by 1 level.
  • In FOR-cycles and IF-branches, stuff is indented one more level than "FOR" or "IF"
  • Continuation line has the same indentation as the line which the continuation line "extends".
  • Lines containing only comments are indented either like the preceding, or the following non-empty line.
  • (Is there any other case influencing indentation?)

I do not know whether Robocop is ready for such a rule, but it is able to work with indentation even now, so I hope so...

@MoreFamed
Copy link
Author

I am sorry I create many issues today, but I am carefully going through our code standard and thinking how Robocop could help us to observe it. 😄

@bhirsz
Copy link
Member

bhirsz commented Sep 17, 2021

I've spent quite a lot of time with indents in Robocop and the reason why it isn't so strict is because how flexible Robot Framework is. Some people prefer different ways of aligning (ie align to columns, or extra align for particular statements). It makes it harder to catch actual issues. That's why current uneven indent rule does not check if we're matching fixed indent but only if all indents are the same in given block (test, keyword, for or if).

But I have an idea: we could add parametr strict (default False). In strict mode we could check for indents in the way you described. And it's actually easier to implement than non strict mode. What do you think?

@bhirsz
Copy link
Member

bhirsz commented Sep 17, 2021

I am sorry I create many issues today, but I am carefully going through our code standard and thinking how Robocop could help us to observe it. 😄

Don't be sorry, we really apprecieate all new ideas or issues how so we can improve Robocop further ;)

@mnojek
Copy link
Member

mnojek commented Sep 17, 2021

I've spent quite a lot of time with indents in Robocop and the reason why it isn't so strict is because how flexible Robot Framework is. Some people prefer different ways of aligning (ie align to columns, or extra align for particular statements). It makes it harder to catch actual issues. That's why current uneven indent rule does not check if we're matching fixed indent but only if all indents are the same in given block (test, keyword, for or if).

But I have an idea: we could add parametr strict (default False). In strict mode we could check for indents in the way you described. And it's actually easier to implement than non strict mode. What do you think?

@bhirsz I totally support this idea.

I suggest to implement all-catching check of indentation. The current rules regarding indentation ([W1007] uneven-indent and [E1008] bad-indent) allow me to indent very sloppily. 😄 It could work roughly like this:

  • One level of indentation is either one tab or 4 spaces (this "4" would be default, but configurable)
  • Names of keywords and test cases in definition are not indented. Their bodies are indented by 1 level.
  • In FOR-cycles and IF-branches, stuff is indented one more level than "FOR" or "IF"
  • Continuation line has the same indentation as the line which the continuation line "extends".
  • Lines containing only comments are indented either like the preceding, or the following non-empty line.
  • (Is there any other case influencing indentation?)

I do not know whether Robocop is ready for such a rule, but it is able to work with indentation even now, so I hope so...

@MoreFamed At least one thing is missing here... What would be the indentation for the first token after continuation line? It can be either next 4 spaces or... something else.

@MoreFamed
Copy link
Author

Hi, thanks for you attitude and ideas! To be clear: by word "indentation", I mean just amount of whitespace at the beginning of a line, not whitespace between tokens. (The latter could hardly be checked because of the reason @bhirsz mentions, if I caught the mention about "aligning to columns" well.)

@mnojek, I am not sure I understand your question. If you ask about space between "..." and the next thing on the line: I do not know. :) I consider this question to be similar to question of spaces between two tokens and I think this is beyond the scope of indentation rules. I can imagine this

    [Documentation]  Some description 
    ...              which is very long

and this too:

    [Documentation]  
    ...  Some description 
    ...  which is very long

I would rather check just the whitespaces before "...".

@mnojek
Copy link
Member

mnojek commented Sep 17, 2021

@MoreFamed You're right, these are 2 different things and I mixed them up.

@mnojek mnojek added this to the 2.0.0 milestone Feb 13, 2022
@bhirsz bhirsz modified the milestones: 2.0.0, 2.1.0 Mar 15, 2022
@bhirsz bhirsz removed this from the 2.1.0 milestone Jul 27, 2022
@bhirsz
Copy link
Member

bhirsz commented Sep 15, 2022

I have starting working on this. There is one issue to be resolved - we have two rules for indentation, uneven-indent and bad-indent. If we would add new "strict" parameter we would need to add it to both rules and then somehow make it work with current logic (since it's "embedded" in current fuzzy indent match logic).

However I'm thinking about leaving only one rule. Just by the description of the rules it may not be clear why there are two rules:
uneven-indent:

       Reported when line does not follow indent from the current block. 
        Example of rule violation::
        
            Keyword With Over Indented Setting
                [Documentation]  this is doc
                 [Arguments]  ${arg}  # over-indented
                   No Operation  # over-indented
                Pass
                No Operation
                Fail

bad-indent:

        Expecting indent. Example of rule violation::
        
             FOR  ${elem}  IN  ${list}
            Log  stuff  # content of FOR blocks should use bigger indentation than FOR header
             END

It made sense to create two rules before because sometimes the indentation is just uneven (hence rule with the warning) and sometimes it is wrong and causes syntax issues (hence rule with the error). But now we have feature of allowing dynamic severity of the rule - so we can use one rule and change severity depending on our needs. For example use bad-indent rule with default severity Warning and optional parameter strict.

Such change will require deprecation warning - but it will not be breaking change (thanks to deprecation warnings) so the next version will be still minor.

@mnojek WDYT?

@bhirsz
Copy link
Member

bhirsz commented Sep 15, 2022

Now I see that the issue is from 17.09.2021 so I have exactly two days before anniversay :D

@mnojek
Copy link
Member

mnojek commented Sep 16, 2022

Actually, the anniversary is today (from the first release)! 😄
Happy birthday, Robocop! 👮🏻‍♂️

So you mean that this one new rule will trigger warning when it will be something like old uneven-indent and it will raise error when it is something like old bad-indent?

@bhirsz
Copy link
Member

bhirsz commented Sep 16, 2022

🥳

Yes. It it still not perfect (the idea that severity can change) but it will be easier to understand by users rather than two existing rules.

@mnojek
Copy link
Member

mnojek commented Sep 16, 2022

I agree, let's do it. These 2 rules were confusing anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants