Repository: MarketSquare/robotframework-robocop
Pull request #1018 (2023-12-25 18:19:15)

Handle VAR syntax in variable naming rules
Relates #973

It does only handle variable naming rules - other rules (like unused-variable or any rule related to variable) still does not support VAR.

codecov[bot]: ## [Codecov](https://app.codecov.io/gh/MarketSquare/robotframework-robocop/pull/1018?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=MarketSquare) Report
Attention: `13 lines` in your changes are missing coverage. Please review.
> Comparison is base [(`03e2704`)](https://app.codecov.io/gh/MarketSquare/robotframework-robocop/commit/03e2704ae5753d0eb5ad233ab800b889bcd73091?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=MarketSquare) 96.66% compared to head [(`1ea7bd1`)](https://app.codecov.io/gh/MarketSquare/robotframework-robocop/pull/1018?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=MarketSquare) 96.29%.

| [Files](https://app.codecov.io/gh/MarketSquare/robotframework-robocop/pull/1018?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=MarketSquare) | Patch % | Lines |
|---|---|---|
| [robocop/checkers/naming.py](https://app.codecov.io/gh/MarketSquare/robotframework-robocop/pull/1018?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=MarketSquare#diff-cm9ib2NvcC9jaGVja2Vycy9uYW1pbmcucHk=) | 38.09% | [13 Missing :warning: ](https://app.codecov.io/gh/MarketSquare/robotframework-robocop/pull/1018?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=MarketSquare) |

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


```diff
@@            Coverage Diff             @@
##           master    #1018      +/-   ##
==========================================
- Coverage   96.66%   96.29%   -0.37%     
==========================================
  Files          37       37              
  Lines        4521     4539      +18     
==========================================
+ Hits         4370     4371       +1     
- Misses        151      168      +17     
```



</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/MarketSquare/robotframework-robocop/pull/1018?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).


Patch:
diff --git a/docs/releasenotes/unreleased/rules.1.rst b/docs/releasenotes/unreleased/rules.1.rst
index e69de29..a39beab 100644
--- a/docs/releasenotes/unreleased/rules.1.rst
+++ b/docs/releasenotes/unreleased/rules.1.rst
@@ -0,0 +1,8 @@
+Updated existing rules with VAR support (#973)
+-----------------------------------------------
+
+Following rules now support ``VAR`` syntax:
+
+- W0310 ``non-local-variables-should-be-uppercase``
+- I0317 ``hyphen-in-variable-name``
+- W0324 ``overwriting-reserved-variable``
diff --git a/robocop/checkers/naming.py b/robocop/checkers/naming.py
index c9b5d25..ca56b40 100644
--- a/robocop/checkers/naming.py
+++ b/robocop/checkers/naming.py
@@ -596,7 +596,7 @@ class InvalidCharactersInNameChecker(VisitorChecker):
             # Loop and skip variables:
             # Search pattern from start_pos to variable starting position
             # example `Keyword With ${em.bedded} Two ${second.Argument} Argument``
-            # is splitted to:
+            # is split to:
             #   1. `Keyword With `
             #   2. ` Two `
             #   3. ` Argument` - last part is searched in finditer part after this loop
@@ -981,7 +981,6 @@ class VariableNamingChecker(VisitorChecker):
     def visit_KeywordCall(self, node):  # noqa
         for token in node.get_tokens(Token.ASSIGN):
             self.check_for_reserved_naming_or_hyphen(token, "Variable", is_assign=True)
-
         if not node.keyword:
             return
         if normalize_robot_name(node.keyword, remove_prefix="builtin.") in SET_VARIABLE_VARIANTS:
@@ -996,16 +995,38 @@ class VariableNamingChecker(VisitorChecker):
                 return  # TODO: Ignore for now, for example ${not  closed in variables will throw it
             if var_name is None:  # possibly $escaped or \${escaped}, or invalid variable name
                 return
-            normalized_var_name = remove_nested_variables(var_name)
-            # a variable as a keyword argument can contain lowercase nested variable
-            # because the actual value of it may be uppercase
-            if not normalized_var_name.isupper():
-                self.report(
-                    "non-local-variables-should-be-uppercase",
-                    node=node,
-                    col=token.col_offset + 1,
-                    end_col=token.end_col_offset + 1,
-                )
+            self.check_non_local_variable(var_name, node, token)
+
+    def check_non_local_variable(self, variable_name: str, node, token):
+        normalized_var_name = remove_nested_variables(variable_name)
+        # a variable as a keyword argument can contain lowercase nested variable
+        # because the actual value of it may be uppercase
+        if not normalized_var_name.isupper():
+            self.report(
+                "non-local-variables-should-be-uppercase",
+                node=node,
+                col=token.col_offset + 1,
+                end_col=token.end_col_offset + 1,
+            )
+
+    @staticmethod
+    def _is_var_scope_local(node):
+        is_local = True
+        for option in node.get_tokens(Token.OPTION):
+            if "scope=" in option.value:
+                is_local = option.value.lower() == "scope=local"
+        return is_local
+
+    def visit_Var(self, node):  # noqa
+        if node.errors:  # for example invalid variable definition like $var}
+            return
+        variable = node.get_token(Token.VARIABLE)
+        if not variable:
+            return
+        self.check_for_reserved_naming_or_hyphen(variable, "Variable", is_assign=True)
+        # TODO Check supported syntax for variable, ie ${{var}}?
+        if not self._is_var_scope_local(node):
+            self.check_non_local_variable(variable.value, node, variable)
 
     def visit_If(self, node):  # noqa
         for token in node.header.get_tokens(Token.ASSIGN):


Test patch:
diff --git a/tests/atest/rules/naming/hyphen_in_variable_name/VAR_syntax.robot b/tests/atest/rules/naming/hyphen_in_variable_name/VAR_syntax.robot
new file mode 100644
index 0000000..a5bd494
--- /dev/null
+++ b/tests/atest/rules/naming/hyphen_in_variable_name/VAR_syntax.robot
@@ -0,0 +1,8 @@
+*** Test Cases ***
+My Test Case
+    VAR    ${r}   ${2-1}  # this is fine
+    VAR    ${a-b}    1  # this will warn - because if it's later used as ${a-b} it can lead to ambiguous results
+    VAR    ${a\-b}  1  # this will warn
+    VAR    ${-}    1   scope=GLOBAL  # this will warn
+    VAR    ${a-}   1   # this will warn
+    VAR    ${-b}   1   # this will warn
diff --git a/tests/atest/rules/naming/hyphen_in_variable_name/expected_output_var.txt b/tests/atest/rules/naming/hyphen_in_variable_name/expected_output_var.txt
new file mode 100644
index 0000000..eab7dcd
--- /dev/null
+++ b/tests/atest/rules/naming/hyphen_in_variable_name/expected_output_var.txt
@@ -0,0 +1,5 @@
+VAR_syntax.robot:4:12 [I] 0317 Use underscore in variable name '${a-b}' instead of hyphens to avoid treating them like minus sign
+VAR_syntax.robot:5:12 [I] 0317 Use underscore in variable name '${a\-b}' instead of hyphens to avoid treating them like minus sign
+VAR_syntax.robot:6:12 [I] 0317 Use underscore in variable name '${-}' instead of hyphens to avoid treating them like minus sign
+VAR_syntax.robot:7:12 [I] 0317 Use underscore in variable name '${a-}' instead of hyphens to avoid treating them like minus sign
+VAR_syntax.robot:8:12 [I] 0317 Use underscore in variable name '${-b}' instead of hyphens to avoid treating them like minus sign
\ No newline at end of file
diff --git a/tests/atest/rules/naming/hyphen_in_variable_name/test_rule.py b/tests/atest/rules/naming/hyphen_in_variable_name/test_rule.py
index 6f32193..4458464 100644
--- a/tests/atest/rules/naming/hyphen_in_variable_name/test_rule.py
+++ b/tests/atest/rules/naming/hyphen_in_variable_name/test_rule.py
@@ -4,3 +4,6 @@ from tests.atest.utils import RuleAcceptance
 class TestRuleAcceptance(RuleAcceptance):
     def test_rule(self):
         self.check_rule(src_files=["test.robot"], expected_file="expected_output.txt")
+
+    def test_var_syntax(self):
+        self.check_rule(src_files=["VAR_syntax.robot"], expected_file="expected_output_var.txt", target_version=">=7")
diff --git a/tests/atest/rules/naming/non_local_variables_should_be_uppercase/VAR_syntax.robot b/tests/atest/rules/naming/non_local_variables_should_be_uppercase/VAR_syntax.robot
new file mode 100644
index 0000000..5a7a93c
--- /dev/null
+++ b/tests/atest/rules/naming/non_local_variables_should_be_uppercase/VAR_syntax.robot
@@ -0,0 +1,10 @@
+*** Keywords ***
+VAR syntax
+    VAR    ${suite}    value    scope=SUITE
+    VAR    ${global}    value    scope=GLOBAL
+    VAR    ${test}    value    scope=TEST
+    VAR    ${task}    value    scope=TASK
+    VAR    ${local_default}    value    scope=local
+    VAR    ${local_default}    value
+    VAR    ${invalid
+
diff --git a/tests/atest/rules/naming/non_local_variables_should_be_uppercase/expected_output_var.txt b/tests/atest/rules/naming/non_local_variables_should_be_uppercase/expected_output_var.txt
new file mode 100644
index 0000000..14ac573
--- /dev/null
+++ b/tests/atest/rules/naming/non_local_variables_should_be_uppercase/expected_output_var.txt
@@ -0,0 +1,4 @@
+VAR_syntax.robot:3:12 [W] 0310 Test, suite and global variables should be uppercase
+VAR_syntax.robot:4:12 [W] 0310 Test, suite and global variables should be uppercase
+VAR_syntax.robot:5:12 [W] 0310 Test, suite and global variables should be uppercase
+VAR_syntax.robot:6:12 [W] 0310 Test, suite and global variables should be uppercase
\ No newline at end of file
diff --git a/tests/atest/rules/naming/non_local_variables_should_be_uppercase/test_rule.py b/tests/atest/rules/naming/non_local_variables_should_be_uppercase/test_rule.py
index 6f32193..eb0b7b2 100644
--- a/tests/atest/rules/naming/non_local_variables_should_be_uppercase/test_rule.py
+++ b/tests/atest/rules/naming/non_local_variables_should_be_uppercase/test_rule.py
@@ -4,3 +4,6 @@ from tests.atest.utils import RuleAcceptance
 class TestRuleAcceptance(RuleAcceptance):
     def test_rule(self):
         self.check_rule(src_files=["test.robot"], expected_file="expected_output.txt")
+
+    def test_var(self):
+        self.check_rule(src_files=["VAR_syntax.robot"], expected_file="expected_output_var.txt", target_version=">=7")
diff --git a/tests/atest/rules/naming/overwriting_reserved_variable/VAR_syntax.robot b/tests/atest/rules/naming/overwriting_reserved_variable/VAR_syntax.robot
new file mode 100644
index 0000000..79ee792
--- /dev/null
+++ b/tests/atest/rules/naming/overwriting_reserved_variable/VAR_syntax.robot
@@ -0,0 +1,12 @@
+*** Test Cases ***
+Overwrite reserved with VAR
+    VAR     ${TEST_NAME}    new_value
+    VAR    ${TEST DOCUMENTATION}    new value    scope=GLOBAL
+    VAR    ${LOG LEVEL}    ${OPTIONS}    scope=LOCAL
+
+
+*** Keywords ***
+Overwrite reserved with VAR
+    VAR     ${TEST_NAME}    new_value
+    VAR    ${TEST DOCUMENTATION}    new value    scope=GLOBAL
+    VAR    ${LOG LEVEL}    ${OPTIONS}    scope=LOCAL
diff --git a/tests/atest/rules/naming/overwriting_reserved_variable/expected_output_var.txt b/tests/atest/rules/naming/overwriting_reserved_variable/expected_output_var.txt
new file mode 100644
index 0000000..82323a6
--- /dev/null
+++ b/tests/atest/rules/naming/overwriting_reserved_variable/expected_output_var.txt
@@ -0,0 +1,6 @@
+VAR_syntax.robot:3:13 [W] 0324 Variable '${TEST_NAME}' overwrites reserved variable '${TEST_NAME}'
+VAR_syntax.robot:4:12 [W] 0324 Variable '${TEST DOCUMENTATION}' overwrites reserved variable '${TEST_DOCUMENTATION}'
+VAR_syntax.robot:5:12 [W] 0324 Variable '${LOG LEVEL}' overwrites reserved variable '${LOG_LEVEL}'
+VAR_syntax.robot:10:13 [W] 0324 Variable '${TEST_NAME}' overwrites reserved variable '${TEST_NAME}'
+VAR_syntax.robot:11:12 [W] 0324 Variable '${TEST DOCUMENTATION}' overwrites reserved variable '${TEST_DOCUMENTATION}'
+VAR_syntax.robot:12:12 [W] 0324 Variable '${LOG LEVEL}' overwrites reserved variable '${LOG_LEVEL}'
\ No newline at end of file
diff --git a/tests/atest/rules/naming/overwriting_reserved_variable/test_rule.py b/tests/atest/rules/naming/overwriting_reserved_variable/test_rule.py
index 6abb757..eeb46cb 100644
--- a/tests/atest/rules/naming/overwriting_reserved_variable/test_rule.py
+++ b/tests/atest/rules/naming/overwriting_reserved_variable/test_rule.py
@@ -4,3 +4,6 @@ from tests.atest.utils import RuleAcceptance
 class TestRuleAcceptance(RuleAcceptance):
     def test_rule(self):
         self.check_rule(src_files=["test.robot"], expected_file="expected_output.txt", issue_format="end_col")
+
+    def test_var(self):
+        self.check_rule(src_files=["VAR_syntax.robot"], expected_file="expected_output_var.txt", target_version=">=7")