Lint: Add a check for indentation being 4N spaces. (#5772)

* Add a check for indentation being 4N spaces.
Make 'url' key optional in check specification.
Document pyproject.toml fields.

* added changelog; fixed tests

* test indentations

* fix test to use real url

* allow -n to set 'S012'.

* response to review

* remove unwanted import

* review response: Move whitespace/newlines into rst only section

* refix mypy import issue

Author: wxtim

changes.d/5772.feat.md

@@ -0,0 +1 @@
+Add a check for indentation being 4N spaces.

cylc/flow/scripts/lint.py

@@ -22,6 +22,8 @@
 # NOTE: docstring needed for `cylc help all` output
 # (if editing check this still comes out as expected)
 
+LINT_SECTIONS = ['cylc-lint', 'cylclint', 'cylc_lint']
+
 COP_DOC = """cylc lint [OPTIONS] ARGS
 
 Check .cylc and .rc files for code style, deprecated syntax and other issues.
@@ -33,13 +35,18 @@
 
 A non-zero return code will be returned if any issues are identified.
 This can be overridden by providing the "--exit-zero" flag.
+"""
 
-Configurations for Cylc lint can also be set in a pyproject.toml file.
-
+TOMLDOC = """
+pyproject.toml configuration:{}
+   [cylc-lint]                     # any of {}
+       ignore = ['S001', 'S002]    # List of rules to ignore
+       exclude = ['etc/foo.cylc']  # List of files to ignore
+       rulesets = ['style', '728'] # Sets default rulesets to check
+       max-line-length = 130       # Max line length for linting
 """
 from colorama import Fore
 import functools
-from optparse import Values
 from pathlib import Path
 import re
 import sys
@@ -59,7 +66,7 @@
         loads as toml_loads,
         TOMLDecodeError,
     )
-from typing import Callable, Dict, Iterator, List, Union
+from typing import TYPE_CHECKING, Callable, Dict, Iterator, List, Union
 
 from cylc.flow import LOG
 from cylc.flow.exceptions import CylcError
@@ -73,6 +80,10 @@
 from cylc.flow.scripts.cylc import DEAD_ENDS
 from cylc.flow.terminal import cli_function
 
+if TYPE_CHECKING:
+    from optparse import Values
+
+
 DEPRECATED_ENV_VARS = {
     'CYLC_SUITE_HOST': 'CYLC_WORKFLOW_HOST',
     'CYLC_SUITE_OWNER': 'CYLC_WORKFLOW_OWNER',
@@ -222,13 +233,44 @@ def check_for_obsolete_environment_variables(line: str) -> List[str]:
     return [i for i in OBSOLETE_ENV_VARS if i in line]
 
 
+INDENTATION = re.compile(r'^(\s*)(.*)')
+
+
+def check_indentation(line: str) -> bool:
+    """The key value pair is not indented 4*X spaces
+
+    n.b. We test for trailing whitespace and incorrect section indenting
+    elsewhere
+
+    Examples:
+
+        >>> check_indentation('')
+        False
+        >>> check_indentation('   ')
+        False
+        >>> check_indentation('   [')
+        False
+        >>> check_indentation('baz')
+        False
+        >>> check_indentation('    qux')
+        False
+        >>> check_indentation('   foo')
+        True
+        >>> check_indentation('     bar')
+        True
+    """
+    match = INDENTATION.findall(line)[0]
+    if not match[0] or not match[1] or match[1].startswith('['):
+        return False
+    return bool(len(match[0]) % 4 != 0)
+
+
 FUNCTION = 'function'
 
 STYLE_GUIDE = (
     'https://cylc.github.io/cylc-doc/stable/html/workflow-design-guide/'
     'style-guide.html#'
 )
-URL_STUB = "https://cylc.github.io/cylc-doc/stable/html/7-to-8/"
 SECTION2 = r'\[\[\s*{}\s*\]\]'
 SECTION3 = r'\[\[\[\s*{}\s*\]\]\]'
 FILEGLOBS = ['*.rc', '*.cylc']
@@ -268,7 +310,6 @@ def check_for_obsolete_environment_variables(line: str) -> List[str]:
 # - short: A short description of the issue.
 # - url: A link to a fuller description.
 # - function: A function to use to run the check.
-# - fallback: A second function(The first function might want to call this?)
 # - kwargs: We want to pass a set of common kwargs to the check function.
 # - evaluate commented lines: Run this check on commented lines.
 # - rst: An rst description, for use in the Cylc docs.
@@ -398,21 +439,32 @@ def check_for_obsolete_environment_variables(line: str) -> List[str]:
             check_if_jinja2,
             function=re.compile(r'(?<!{)#.*?{[{%]').findall
         )
+    },
+    'S012': {
+        'short': 'This number is reserved for line length checks',
+    },
+    'S013': {
+        'short': 'Items should be indented in 4 space blocks.',
+        FUNCTION: check_indentation
     }
 }
 # Subset of deprecations which are tricky (impossible?) to scrape from the
 # upgrader.
 MANUAL_DEPRECATIONS = {
     "U001": {
-        'short': DEPENDENCY_SECTION_MSG['text'],
+        'short': (
+            DEPENDENCY_SECTION_MSG['text'] + ' (``[dependencies]`` detected)'
+        ),
         'url': '',
-        'rst': DEPENDENCY_SECTION_MSG['rst'],
+        'rst': (
+            DEPENDENCY_SECTION_MSG['rst'] + ' (``[dependencies]`` detected)'
+        ),
         FUNCTION: re.compile(SECTION2.format('dependencies')).findall,
     },
     "U002": {
-        'short': DEPENDENCY_SECTION_MSG['text'],
+        'short': DEPENDENCY_SECTION_MSG['text'] + ' (``graph =`` detected)',
         'url': '',
-        'rst': DEPENDENCY_SECTION_MSG['rst'],
+        'rst': DEPENDENCY_SECTION_MSG['rst'] + ' (``graph =`` detected)',
         FUNCTION: re.compile(r'graph\s*=\s*').findall,
     },
     "U003": {
@@ -541,6 +593,31 @@ def check_for_obsolete_environment_variables(line: str) -> List[str]:
 }
 
 
+def get_url(check_meta: Dict) -> str:
+    """Get URL from check data.
+
+    If the URL doesn't start with http then prepend with address
+    of the 7-to-8 upgrade guide.
+
+    Examples:
+        >>> get_url({'no': 'url key'})
+        ''
+        >>> get_url({'url': ''})
+        ''
+        >>> get_url({'url': 'https://www.h2g2.com/'})
+        'https://www.h2g2.com/'
+        >>> get_url({'url': 'cheat-sheet.html'})
+        'https://cylc.github.io/cylc-doc/stable/html/7-to-8/cheat-sheet.html'
+    """
+    url = check_meta.get('url', '')
+    if url and not url.startswith('http'):
+        url = (
+            "https://cylc.github.io/cylc-doc/stable/html/7-to-8/"
+            + check_meta['url']
+        )
+    return url
+
+
 def validate_toml_items(tomldata):
     """Check that all tomldata items are lists of strings
 
@@ -590,7 +667,7 @@ def get_pyproject_toml(dir_):
             raise CylcError(f'pyproject.toml did not load: {exc}')
 
         if any(
-            i in loadeddata for i in ['cylc-lint', 'cylclint', 'cylc_lint']
+            i in loadeddata for i in LINT_SECTIONS
         ):
             for key in keys:
                 tomldata[key] = loadeddata.get('cylc-lint').get(key, [])
@@ -906,10 +983,7 @@ def lint(
                 counter[check_meta['purpose']] += 1
                 if modify:
                     # insert a command to help the user
-                    if check_meta['url'].startswith('http'):
-                        url = check_meta['url']
-                    else:
-                        url = URL_STUB + check_meta['url']
+                    url = get_url(check_meta)
 
                     yield (
                         f'# [{get_index_str(check_meta, index)}]: '
@@ -983,10 +1057,7 @@ def get_reference_rst(checks):
             template = (
                 '{check}\n^^^^\n{summary}\n\n'
             )
-            if meta['url'].startswith('http'):
-                url = meta['url']
-            else:
-                url = URL_STUB + meta['url']
+            url = get_url(meta)
             summary = meta.get("rst", meta['short'])
             msg = template.format(
                 check=get_index_str(meta, index),
@@ -1027,10 +1098,7 @@ def get_reference_text(checks):
             template = (
                 '{check}:\n    {summary}\n\n'
             )
-            if meta['url'].startswith('http'):
-                url = meta['url']
-            else:
-                url = URL_STUB + meta['url']
+            url = get_url(meta)
             msg = template.format(
                 title=index,
                 check=get_index_str(meta, index),
@@ -1044,7 +1112,7 @@ def get_reference_text(checks):
 
 def get_option_parser() -> COP:
     parser = COP(
-        COP_DOC,
+        COP_DOC + TOMLDOC.format('', str(LINT_SECTIONS)),
         argdoc=[
             COP.optional(WORKFLOW_ID_OR_PATH_ARG_DOC)
         ],
@@ -1086,7 +1154,7 @@ def get_option_parser() -> COP:
         default=[],
         dest='ignores',
         metavar="CODE",
-        choices=tuple(STYLE_CHECKS)
+        choices=list(STYLE_CHECKS.keys()) + [LINE_LEN_NO]
     )
     parser.add_option(
         '--exit-zero',
@@ -1204,4 +1272,6 @@ def main(parser: COP, options: 'Values', target=None) -> None:
 
 # NOTE: use += so that this works with __import__
 # (docstring needed for `cylc help all` output)
-__doc__ += get_reference_rst(parse_checks(['728', 'style'], reference=True))
+__doc__ += TOMLDOC.format(
+    '\n\n.. code-block:: toml\n', str(LINT_SECTIONS)) + get_reference_rst(
+    parse_checks(['728', 'style'], reference=True))

tests/unit/scripts/test_lint.py

@@ -142,7 +142,6 @@
     [[and_another_thing]]
         [[[remote]]]
             host = `rose host-select thingy`
-
 """
 
 
@@ -159,6 +158,9 @@
 # {{quix}}
 
 [runtime]
+    [[this_is_ok]]
+      script = echo "this is incorrectly indented"
+
           [[foo]]
         inherit = hello
      [[[job]]]
@@ -572,11 +574,45 @@ def test_invalid_tomlfile(tmp_path):
     'ref, expect',
     [
         [True, 'line > ``<max_line_len>`` characters'],
-        [False, 'line > 130 characters']
+        [False, 'line > 42 characters']
     ]
 )
 def test_parse_checks_reference_mode(ref, expect):
-    result = parse_checks(['style'], reference=ref)
-    key = list(result.keys())[-1]
-    value = result[key]
+    """Add extra explanation of max line legth setting in reference mode.
+    """
+    result = parse_checks(['style'], reference=ref, max_line_len=42)
+    value = result['S012']
     assert expect in value['short']
+
+
+@pytest.mark.parametrize(
+    'spaces, expect',
+    (
+        (0, 'S002'),
+        (1, 'S013'),
+        (2, 'S013'),
+        (3, 'S013'),
+        (4, None),
+        (5, 'S013'),
+        (6, 'S013'),
+        (7, 'S013'),
+        (8, None),
+        (9, 'S013')
+    )
+)
+def test_indents(spaces, expect):
+    """Test different wrong indentations
+
+    Parameterization deliberately over-obvious to avoid replicating
+    arithmetic logic from code. Dangerously close to re-testing ``%``
+    builtin.
+    """
+    result = lint_text(
+        f"{' ' * spaces}foo = 42",
+        ['style']
+    )
+    result = ''.join(result.messages)
+    if expect:
+        assert expect in result
+    else:
+        assert not result