Repository: MarketSquare/robotframework-robocop
Pull request #1088 (2024-06-04 13:30:20)

Add rules checking that builtin and custom libraries are imported in alphabetical order.
Add rules if builtin and custom libraries are sorted alphabetically

bhirsz: Thanks for the good PR. In a way it implements https://github.com/MarketSquare/robotframework-robocop/issues/107 . 
Though there is caveat here: order of the custom imports may be important (and it often is. For example you need to import B before you import A because B keeps variables you need for A). However it should be fine for built in imports. So the solution could be moving those rules (or at least the custom imports one) to community rules (which are disabled by default and can be enabled). What do you think about it?
codecov[bot]: ## [Codecov](https://app.codecov.io/gh/MarketSquare/robotframework-robocop/pull/1088?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=MarketSquare) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Project coverage is 94.64%. Comparing base [(`f62508c`)](https://app.codecov.io/gh/MarketSquare/robotframework-robocop/commit/f62508cff6bcaf0f82139e831e40c601aba5c584?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=MarketSquare) to head [(`d4e2a5a`)](https://app.codecov.io/gh/MarketSquare/robotframework-robocop/commit/d4e2a5a354ce29ea27abfb29befcdae91f71d899?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=MarketSquare).
> Report is 18 commits behind head on master.


<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##           master    #1088      +/-   ##
==========================================
- Coverage   97.02%   94.64%   -2.38%     
==========================================
  Files          37       37              
  Lines        4670     4745      +75     
==========================================
- Hits         4531     4491      -40     
- Misses        139      254     +115     
```



</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/MarketSquare/robotframework-robocop/pull/1088?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=MarketSquare).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=MarketSquare).

szymonslodkowski: @bhirsz thanks for quick review.
So I will leave the rule for builtins in the file I already implemented.
I was not aware about use cases in which order of imports is important - good to know that there are some. So  I will move rule checking rest of imports to custom rules.
Maybe there is also sense to implement rule verifying order of resources? I could try also to add such rule in this PR. Is it ok?
bhirsz: Yeah, feel free to do so - since it will be also custom rule then we can have such rule for resources as well (people can opt in for it).

szymonslodkowski: Ok, but I have a question about community rules numbering.
In robocop/checkers/community_rules there is only keywords.py with `RULE_CATEGORY_ID = "00"` I guess these new rules doesn't fit to this file. I would like to create new one, but what should be RULE_CATEGORY_ID and what should be rule_id?
bhirsz: You can decide it. But since we started with 00, then next one will be 01. Community rules are prefixed with 1 so final rule will be for example 10101, 10102... 

Patch:
diff --git a/robocop/checkers/community_rules/misc.py b/robocop/checkers/community_rules/misc.py
new file mode 100644
index 0000000..bb3314b
--- /dev/null
+++ b/robocop/checkers/community_rules/misc.py
@@ -0,0 +1,100 @@
+from robot.api import Token
+from robot.libraries import STDLIBS
+
+from robocop.checkers import VisitorChecker
+from robocop.rules import Rule, RuleSeverity
+
+RULE_CATEGORY_ID = "01"
+
+
+rules = {
+    "10101": Rule(
+        rule_id="10101",
+        name="non-builtin-imports-not-sorted",
+        msg="Non builtin library import '{{ custom_import }}' should be placed before '{{ previous_custom_import }}'",
+        severity=RuleSeverity.WARNING,
+        added_in_version="5.2.0",
+        enabled=False,
+        docs="""
+        Example of rule violation:
+
+            *** Settings ***
+            Library    Collections
+            Library    CustomLibrary
+            Library    AnotherCustomLibrary  # AnotherCustomLibrary library defined after custom CustomLibrary
+
+        """,
+    ),
+    "10102": Rule(
+        rule_id="10102",
+        name="resources-imports-not-sorted",
+        msg="Resource import '{{ resource_import }}' should be placed before '{{ previous_resource_import }}'",
+        severity=RuleSeverity.WARNING,
+        added_in_version="5.2.0",
+        enabled=False,
+        docs="""
+        Example of rule violation:
+
+            *** Settings ***
+            Resource   CustomResource.resource
+            Resource   AnotherFile.resource
+
+        """,
+    ),
+}
+
+
+class NonBuiltinLibrariesImportOrderChecker(VisitorChecker):
+    """
+    Find and report Non Builtin Libraries or Resources imported not in alphabetical order.
+    """
+
+    reports = (
+        "non-builtin-imports-not-sorted",
+        "resources-imports-not-sorted",
+    )
+
+    def __init__(self):
+        self.non_builtin_libraries = []
+        self.resources = []
+        super().__init__()
+
+    def visit_File(self, node):  # noqa
+        self.non_builtin_libraries = []
+        self.resources = []
+        self.generic_visit(node)
+        previous = None
+        for library in self.non_builtin_libraries:
+            if previous is not None and library.name < previous.name:
+                lib_name = library.get_token(Token.NAME)
+                self.report(
+                    "non-builtin-imports-not-sorted",
+                    custom_import=library.name,
+                    previous_custom_import=previous.name,
+                    node=library,
+                    col=lib_name.col_offset + 1,
+                    end_col=lib_name.end_col_offset + 1,
+                )
+            previous = library
+        previous = None
+        for resource in self.resources:
+            if previous is not None and resource.name < previous.name:
+                resource_name = resource.get_token(Token.NAME)
+                self.report(
+                    "resources-imports-not-sorted",
+                    resource_import=resource.name,
+                    previous_resource_import=previous.name,
+                    node=resource,
+                    col=resource_name.col_offset + 1,
+                    end_col=resource_name.end_col_offset + 1,
+                )
+            previous = resource
+
+    def visit_LibraryImport(self, node):  # noqa
+        if node.name and node.name not in STDLIBS:
+            self.non_builtin_libraries.append(node)
+
+    def visit_ResourceImport(self, node):  # noqa
+        if not node.name:
+            return
+        self.resources.append(node)
diff --git a/robocop/checkers/misc.py b/robocop/checkers/misc.py
index e2da669..16b5e25 100644
--- a/robocop/checkers/misc.py
+++ b/robocop/checkers/misc.py
@@ -586,6 +586,21 @@ rules = {
         """,
         added_in_version="4.0.0",
     ),
+    "0926": Rule(
+        rule_id="0926",
+        name="builtin-imports-not-sorted",
+        msg="BuiltIn library import '{{ builtin_import }}' should be placed before '{{ previous_builtin_import }}'",
+        severity=RuleSeverity.WARNING,
+        added_in_version="5.2.0",
+        docs="""
+        Example of rule violation::
+
+            *** Settings ***
+            Library    OperatingSystem
+            Library    Collections  # BuiltIn libraries imported not in alphabetical order
+
+        """,
+    ),
 }
 
 
@@ -796,7 +811,10 @@ class SettingsOrderChecker(VisitorChecker):
     BuiltIn libraries imports should always be placed before other libraries imports.
     """
 
-    reports = ("wrong-import-order",)
+    reports = (
+        "wrong-import-order",
+        "builtin-imports-not-sorted",
+    )
 
     def __init__(self):
         self.libraries = []
@@ -806,6 +824,7 @@ class SettingsOrderChecker(VisitorChecker):
         self.libraries = []
         self.generic_visit(node)
         first_non_builtin = None
+        previous_builtin = None
         for library in self.libraries:
             if first_non_builtin is None:
                 if library.name not in STDLIBS:
@@ -821,6 +840,18 @@ class SettingsOrderChecker(VisitorChecker):
                         col=lib_name.col_offset + 1,
                         end_col=lib_name.end_col_offset + 1,
                     )
+            if library.name in STDLIBS:
+                if previous_builtin is not None and library.name < previous_builtin.name:
+                    lib_name = library.get_token(Token.NAME)
+                    self.report(
+                        "builtin-imports-not-sorted",
+                        builtin_import=library.name,
+                        previous_builtin_import=previous_builtin.name,
+                        node=library,
+                        col=lib_name.col_offset + 1,
+                        end_col=lib_name.end_col_offset + 1,
+                    )
+                previous_builtin = library
 
     def visit_LibraryImport(self, node):  # noqa
         if not node.name:


Test patch:
diff --git a/tests/atest/rules/community_rules/misc/non_builtin_imports_not_sorted/__init__.py b/tests/atest/rules/community_rules/misc/non_builtin_imports_not_sorted/__init__.py
new file mode 100644
index 0000000..e69de29
diff --git a/tests/atest/rules/community_rules/misc/non_builtin_imports_not_sorted/expected_output.txt b/tests/atest/rules/community_rules/misc/non_builtin_imports_not_sorted/expected_output.txt
new file mode 100644
index 0000000..6b2ff66
--- /dev/null
+++ b/tests/atest/rules/community_rules/misc/non_builtin_imports_not_sorted/expected_output.txt
@@ -0,0 +1,2 @@
+no_builtins.robot:4:12 [W] 10101 Non builtin library import 'OwnLib.py' should be placed before 'RequestsLibrary'
+test.robot:9:12 [W] 10101 Non builtin library import 'AnotherLibrary' should be placed before 'RequestsLibrary'
\ No newline at end of file
diff --git a/tests/atest/rules/community_rules/misc/non_builtin_imports_not_sorted/no_builtins.robot b/tests/atest/rules/community_rules/misc/non_builtin_imports_not_sorted/no_builtins.robot
new file mode 100644
index 0000000..a4d1cac
--- /dev/null
+++ b/tests/atest/rules/community_rules/misc/non_builtin_imports_not_sorted/no_builtins.robot
@@ -0,0 +1,11 @@
+*** Settings ***
+Library    RequestsLibrary
+Resource   ..${/}Libs${/}MyResource.robot
+Library    OwnLib.py
+Variables    variables.py
+Test Template    Keyword
+
+
+*** Keywords ***
+Keyword
+    No Operation
diff --git a/tests/atest/rules/community_rules/misc/non_builtin_imports_not_sorted/no_imports.robot b/tests/atest/rules/community_rules/misc/non_builtin_imports_not_sorted/no_imports.robot
new file mode 100644
index 0000000..8b3fbab
--- /dev/null
+++ b/tests/atest/rules/community_rules/misc/non_builtin_imports_not_sorted/no_imports.robot
@@ -0,0 +1,9 @@
+*** Settings ***
+Resource   ..${/}Libs${/}MyResource.robot
+Variables    variables.py
+Test Template    Keyword
+
+
+*** Keywords ***
+Keyword
+    No Operation
diff --git a/tests/atest/rules/community_rules/misc/non_builtin_imports_not_sorted/only_builtins.robot b/tests/atest/rules/community_rules/misc/non_builtin_imports_not_sorted/only_builtins.robot
new file mode 100644
index 0000000..464a2bb
--- /dev/null
+++ b/tests/atest/rules/community_rules/misc/non_builtin_imports_not_sorted/only_builtins.robot
@@ -0,0 +1,12 @@
+*** Settings ***
+Library    Collections
+Library    String
+Resource   ..${/}Libs${/}MyResource.robot
+Variables    variables.py
+Test Template    Keyword
+Library    BuiltIn
+
+
+*** Keywords ***
+Keyword
+    No Operation
diff --git a/tests/atest/rules/community_rules/misc/non_builtin_imports_not_sorted/test.robot b/tests/atest/rules/community_rules/misc/non_builtin_imports_not_sorted/test.robot
new file mode 100644
index 0000000..cef9b9d
--- /dev/null
+++ b/tests/atest/rules/community_rules/misc/non_builtin_imports_not_sorted/test.robot
@@ -0,0 +1,18 @@
+*** Settings ***
+Library    DateTime
+Library    Collections
+Library    OperatingSystem
+Library    XML
+Library    String
+Library    OwnLib.py
+Library    RequestsLibrary
+Library    AnotherLibrary
+Resource   ..${/}Libs${/}MyResource.robot
+
+Variables    variables.py
+Test Template    Keyword
+
+
+*** Keywords ***
+Keyword
+    No Operation
diff --git a/tests/atest/rules/community_rules/misc/non_builtin_imports_not_sorted/test_rule.py b/tests/atest/rules/community_rules/misc/non_builtin_imports_not_sorted/test_rule.py
new file mode 100644
index 0000000..88377fa
--- /dev/null
+++ b/tests/atest/rules/community_rules/misc/non_builtin_imports_not_sorted/test_rule.py
@@ -0,0 +1,6 @@
+from tests.atest.utils import RuleAcceptance
+
+
+class TestRuleAcceptance(RuleAcceptance):
+    def test_rule(self):
+        self.check_rule(expected_file="expected_output.txt")
diff --git a/tests/atest/rules/community_rules/misc/resources_imports_not_sorted/__init__.py b/tests/atest/rules/community_rules/misc/resources_imports_not_sorted/__init__.py
new file mode 100644
index 0000000..e69de29
diff --git a/tests/atest/rules/community_rules/misc/resources_imports_not_sorted/expected_output.txt b/tests/atest/rules/community_rules/misc/resources_imports_not_sorted/expected_output.txt
new file mode 100644
index 0000000..e8005bc
--- /dev/null
+++ b/tests/atest/rules/community_rules/misc/resources_imports_not_sorted/expected_output.txt
@@ -0,0 +1,2 @@
+test.robot:5:12 [W] 10102 Resource import 'AnotherFile.resource' should be placed before 'CustomResource.resource'
+test.robot:6:12 [W] 10102 Resource import '..\${/}Libs\${/}MyResource.robot' should be placed before 'AnotherFile.resource'
\ No newline at end of file
diff --git a/tests/atest/rules/community_rules/misc/resources_imports_not_sorted/no_resources.robot b/tests/atest/rules/community_rules/misc/resources_imports_not_sorted/no_resources.robot
new file mode 100644
index 0000000..f95b100
--- /dev/null
+++ b/tests/atest/rules/community_rules/misc/resources_imports_not_sorted/no_resources.robot
@@ -0,0 +1,9 @@
+*** Settings ***
+Library    RequestsLibrary
+Variables    variables.py
+Test Template    Keyword
+
+
+*** Keywords ***
+Keyword
+    No Operation
diff --git a/tests/atest/rules/community_rules/misc/resources_imports_not_sorted/test.robot b/tests/atest/rules/community_rules/misc/resources_imports_not_sorted/test.robot
new file mode 100644
index 0000000..56846f0
--- /dev/null
+++ b/tests/atest/rules/community_rules/misc/resources_imports_not_sorted/test.robot
@@ -0,0 +1,14 @@
+*** Settings ***
+Library    DateTime
+Library    RequestsLibrary
+Resource   CustomResource.resource
+Resource   AnotherFile.resource
+Resource   ..${/}Libs${/}MyResource.robot
+
+Variables    variables.py
+Test Template    Keyword
+
+
+*** Keywords ***
+Keyword
+    No Operation
diff --git a/tests/atest/rules/community_rules/misc/resources_imports_not_sorted/test_rule.py b/tests/atest/rules/community_rules/misc/resources_imports_not_sorted/test_rule.py
new file mode 100644
index 0000000..88377fa
--- /dev/null
+++ b/tests/atest/rules/community_rules/misc/resources_imports_not_sorted/test_rule.py
@@ -0,0 +1,6 @@
+from tests.atest.utils import RuleAcceptance
+
+
+class TestRuleAcceptance(RuleAcceptance):
+    def test_rule(self):
+        self.check_rule(expected_file="expected_output.txt")
diff --git a/tests/atest/rules/misc/builtin_imports_not_sorted/__init__.py b/tests/atest/rules/misc/builtin_imports_not_sorted/__init__.py
new file mode 100644
index 0000000..e69de29
diff --git a/tests/atest/rules/misc/builtin_imports_not_sorted/expected_output.txt b/tests/atest/rules/misc/builtin_imports_not_sorted/expected_output.txt
new file mode 100644
index 0000000..0229052
--- /dev/null
+++ b/tests/atest/rules/misc/builtin_imports_not_sorted/expected_output.txt
@@ -0,0 +1,4 @@
+only_builtins.robot:4:12 [W] 0926 BuiltIn library import 'String' should be placed before 'XML'
+only_builtins.robot:8:12 [W] 0926 BuiltIn library import 'BuiltIn' should be placed before 'String'
+test.robot:3:12 [W] 0926 BuiltIn library import 'Collections' should be placed before 'DateTime'
+test.robot:6:12 [W] 0926 BuiltIn library import 'String' should be placed before 'XML'
\ No newline at end of file
diff --git a/tests/atest/rules/misc/builtin_imports_not_sorted/no_builtins.robot b/tests/atest/rules/misc/builtin_imports_not_sorted/no_builtins.robot
new file mode 100644
index 0000000..a4d1cac
--- /dev/null
+++ b/tests/atest/rules/misc/builtin_imports_not_sorted/no_builtins.robot
@@ -0,0 +1,11 @@
+*** Settings ***
+Library    RequestsLibrary
+Resource   ..${/}Libs${/}MyResource.robot
+Library    OwnLib.py
+Variables    variables.py
+Test Template    Keyword
+
+
+*** Keywords ***
+Keyword
+    No Operation
diff --git a/tests/atest/rules/misc/builtin_imports_not_sorted/no_imports.robot b/tests/atest/rules/misc/builtin_imports_not_sorted/no_imports.robot
new file mode 100644
index 0000000..8b3fbab
--- /dev/null
+++ b/tests/atest/rules/misc/builtin_imports_not_sorted/no_imports.robot
@@ -0,0 +1,9 @@
+*** Settings ***
+Resource   ..${/}Libs${/}MyResource.robot
+Variables    variables.py
+Test Template    Keyword
+
+
+*** Keywords ***
+Keyword
+    No Operation
diff --git a/tests/atest/rules/misc/builtin_imports_not_sorted/only_builtins.robot b/tests/atest/rules/misc/builtin_imports_not_sorted/only_builtins.robot
new file mode 100644
index 0000000..7f14f7d
--- /dev/null
+++ b/tests/atest/rules/misc/builtin_imports_not_sorted/only_builtins.robot
@@ -0,0 +1,13 @@
+*** Settings ***
+Library    Collections
+Library    XML
+Library    String
+Resource   ..${/}Libs${/}MyResource.robot
+Variables    variables.py
+Test Template    Keyword
+Library    BuiltIn
+
+
+*** Keywords ***
+Keyword
+    No Operation
diff --git a/tests/atest/rules/misc/builtin_imports_not_sorted/test.robot b/tests/atest/rules/misc/builtin_imports_not_sorted/test.robot
new file mode 100644
index 0000000..e8b8f13
--- /dev/null
+++ b/tests/atest/rules/misc/builtin_imports_not_sorted/test.robot
@@ -0,0 +1,16 @@
+*** Settings ***
+Library    DateTime
+Library    Collections
+Library    OperatingSystem
+Library    XML
+Library    String
+Library    RequestsLibrary
+Resource   ..${/}Libs${/}MyResource.robot
+Library    OwnLib.py
+Variables    variables.py
+Test Template    Keyword
+
+
+*** Keywords ***
+Keyword
+    No Operation
diff --git a/tests/atest/rules/misc/builtin_imports_not_sorted/test_rule.py b/tests/atest/rules/misc/builtin_imports_not_sorted/test_rule.py
new file mode 100644
index 0000000..88377fa
--- /dev/null
+++ b/tests/atest/rules/misc/builtin_imports_not_sorted/test_rule.py
@@ -0,0 +1,6 @@
+from tests.atest.utils import RuleAcceptance
+
+
+class TestRuleAcceptance(RuleAcceptance):
+    def test_rule(self):
+        self.check_rule(expected_file="expected_output.txt")
diff --git a/tests/atest/utils/__init__.py b/tests/atest/utils/__init__.py
index a422890..f9c5510 100644
--- a/tests/atest/utils/__init__.py
+++ b/tests/atest/utils/__init__.py
@@ -46,7 +46,14 @@ def load_expected_file(test_data, expected_file):
         return []
     expected = test_data / expected_file
     with open(expected, encoding="utf-8") as f:
-        return sorted([line.rstrip("\n").replace(r"${/}", os.path.sep) for line in f])
+        return sorted(
+            [
+                re.sub(r"(?<!\\)(?:\\\\)*(\${/})", os.path.sep.replace("\\", "\\\\"), line.rstrip("\n")).replace(
+                    "\\${/}", "${/}"
+                )
+                for line in f
+            ]
+        )
 
 
 def configure_robocop_with_rule(args, runner, rule, path, src_files: Optional[List], format):