Merge pull request #75 from olehermanse/refactoring2

Small fixes and refactorings / improvements

Author: olehermanse

src/cfengine_cli/deptool.py

@@ -260,7 +260,7 @@ def deps_list(self, ref="master"):
         # currently only_deps is generator of space-separated deps,
         # i.e. each item can contain several items, like this:
         # list(only_deps) = ["lcov", "pthreads-w32 libgnurx"]
-        # to "flattern" it we first join using spaces and then split on spaces
+        # to "flatten" it we first join using spaces and then split on spaces
         # in the middle we also do some clean-ups
         only_deps = " ".join(only_deps).replace("libgcc ", "").split(" ")
         # now only_deps looks like this: ["lcov", "pthreads-w32", "libgnurx"]

src/cfengine_cli/dev.py

@@ -25,53 +25,42 @@ def _continue_prompt() -> bool:
     return answer in ("y", "yes")
 
 
-def _expect_repo(repo) -> bool:
+def _expect_repo(repo):
     cwd = os.getcwd()
     if cwd.endswith(repo):
-        return True
+        return
     print(f"Note: This command is intended to be run in the {repo} repo")
     print(f"      https://github.com/cfengine/{repo}")
-    answer = _continue_prompt()
-    return answer
+    if not _continue_prompt():
+        raise UserError(f"Aborted (expected to be in {repo} repo)")
 
 
 def update_dependency_tables() -> int:
-    answer = _expect_repo("buildscripts")
-    if answer:
-        return _update_dependency_tables()
-    return 1
+    _expect_repo("buildscripts")
+    return _update_dependency_tables()
 
 
 def print_dependency_tables(args) -> int:
-    versions = args.versions
-    answer = _expect_repo("buildscripts")
-    if answer:
-        return print_release_dependency_tables(versions)
-    return 1
+    _expect_repo("buildscripts")
+    return print_release_dependency_tables(args.versions)
 
 
 def format_docs(files) -> int:
-    answer = _expect_repo("documentation")
-    if answer:
-        return update_docs(files)
-    return 1
+    _expect_repo("documentation")
+    return update_docs(files)
 
 
 def lint_docs() -> int:
-    answer = _expect_repo("documentation")
-    if answer:
-        return check_docs()
-    return 1
+    _expect_repo("documentation")
+    return check_docs()
 
 
 def generate_release_information(
     omit_download=False, check=False, min_version=None
 ) -> int:
-    answer = _expect_repo("release-information")
-    if answer:
-        generate_release_information_command(omit_download, check, min_version)
-        return 0
-    return 1
+    _expect_repo("release-information")
+    generate_release_information_command(omit_download, check, min_version)
+    return 0
 
 
 def dispatch_dev_subcommand(subcommand, args) -> int:

src/cfengine_cli/docs.py

@@ -346,20 +346,22 @@ def _process_markdown_code_blocks(
                 os.remove(snippet_path)
 
 
-def _run_black(path):
-    print(f"Formatting '{path}' with black...")
+def _run_formatter(tool, args, cwd, install_hint):
+    print(f"Formatting with {tool}...")
     try:
         subprocess.run(
-            ["black", path],
+            [tool, *args],
             capture_output=True,
             text=True,
             check=True,
-            cwd=".",
+            cwd=cwd,
         )
     except:
-        raise UserError(
-            "Encountered an error running black\nInstall: pipx install black"
-        )
+        raise UserError(f"Encountered an error running {tool}\nInstall: {install_hint}")
+
+
+def _run_black(path):
+    _run_formatter("black", [path], ".", "pipx install black")
 
 
 def _run_prettier(path):
@@ -378,21 +380,15 @@ def _run_prettier(path):
             args.append("**.md")
         if not args:
             return
-    try:
-        # Warning: Beware of shell expansion if you try to run this in your terminal.
-        # Wrong: prettier --write **.markdown **.md
-        # Right: prettier --write '**.markdown' '**.md'
-        subprocess.run(
-            ["prettier", "--embedded-language-formatting", "off", "--write", *args],
-            capture_output=True,
-            text=True,
-            check=True,
-            cwd=directory,
-        )
-    except:
-        raise UserError(
-            "Encountered an error running prettier\nInstall: npm install --global prettier"
-        )
+    # Warning: Beware of shell expansion if you try to run this in your terminal.
+    # Wrong: prettier --write **.markdown **.md
+    # Right: prettier --write '**.markdown' '**.md'
+    _run_formatter(
+        "prettier",
+        ["--embedded-language-formatting", "off", "--write", *args],
+        directory,
+        "npm install --global prettier",
+    )
 
 
 def _update_docs_single_arg(path):

src/cfengine_cli/lint.py

@@ -40,6 +40,18 @@
 from cfengine_cli.utils import UserError
 
 LINT_EXTENSIONS = (".cf", ".json")
+DEFAULT_NAMESPACE = "default"
+VARS_TYPES = {
+    "data",
+    "ilist",
+    "int",
+    "real",
+    "rlist",
+    "slist",
+    "string",
+}
+PROMISE_BLOCK_ATTRIBUTES = ("path", "interpreter")
+KNOWN_FAULTY_FUNCTION_DEFS = {"regex_replace"}
 
 
 @dataclass
@@ -130,7 +142,7 @@ class PolicyFile:
     reused when we iterate over it multiple times.
 
     We store filename, raw data (bytes), array of lines, and syntax tree/nodes.
-    This is a but of "duplication", but they are useful for printing nice
+    This is a bit of "duplication", but they are useful for printing nice
     linting errors.
 
     This is intended as a read-only view of the policy file, not to be used for
@@ -140,7 +152,7 @@ class PolicyFile:
     - Whether the file is empty
     - Whether the file has syntax errors
     - Whether the file uses macros (Macros can indicate we need to be less strict)
-    - Things defined and referenced in the file (bundles, bodies, promise types, )
+    - Things defined and referenced in the file (bundles, bodies, promise types)
     """
 
     def __init__(self, filename: str, snippet: Snippet | None = None):
@@ -179,7 +191,7 @@ class State:
     block_name: str | None = None
     promise_type: str | None = None  # "vars" | "files" | "classes" | ... | None
     attribute_name: str | None = None  # "if" | "string" | "slist" | ... | None
-    namespace: str = "default"  # "ns" | "default" | ... |
+    namespace: str = DEFAULT_NAMESPACE  # "ns" | "default" | ... |
     mode: Mode = Mode.NONE
     walking: bool = False
     strict: bool = True
@@ -244,7 +256,7 @@ def start_file(self, policy_file: PolicyFile):
         assert self.mode != Mode.NONE
 
         self.policy_file = policy_file
-        self.namespace = "default"
+        self.namespace = DEFAULT_NAMESPACE
         self.walking = True
 
     def end_file(self) -> None:
@@ -562,24 +574,14 @@ def _lint_node(
         return 1
     if state.promise_type == "vars" and node.type == "promise":
         attribute_nodes = [x for x in node.children if x.type == "attribute"]
-        # Each vars promise must include exactly 1 of these attributes (a value):
-        vars_types = {
-            "data",
-            "ilist",
-            "int",
-            "real",
-            "rlist",
-            "slist",
-            "string",
-        }
         # Attributes are children of a promise, and attribute names are children of attributes
         # Need to iterate inside to find the attribute name (data, ilist, int, etc.)
         value_nodes = []
         for attr in attribute_nodes:
             for child in attr.children:
                 if child.type != "attribute_name":
                     continue
-                if _text(child) in vars_types:
+                if _text(child) in VARS_TYPES:
                     # Ignore the other attributes which are not values
                     value_nodes.append(child)
 
@@ -658,23 +660,17 @@ def _lint_node(
     if (
         state.block_keyword == "promise"
         and node.type == "attribute_name"
-        and state.attribute_name
-        not in (
-            None,
-            "path",
-            "interpreter",
-        )
+        and state.attribute_name not in (None, *PROMISE_BLOCK_ATTRIBUTES)
     ):
         _highlight_range(node, lines)
         print(
             f"Error: Invalid attribute name '{state.attribute_name}' in '{state.block_name}' custom promise type definition {location}"
         )
         return 1
     if node.type == "call":
-        known_faulty_defs = {"regex_replace"}
         call, _, *args, _ = node.children  # f ( a1 , a2 , a..N )
         call = _text(call)
-        if call in known_faulty_defs:
+        if call in KNOWN_FAULTY_FUNCTION_DEFS:
             return 0
 
         args = list(filter(",".__ne__, iter(_text(x) for x in args)))
@@ -986,8 +982,8 @@ def _walk_callback(node: Node, callback: Callable[[Node], int]) -> int:
 def _parse_policy_file(filename: str) -> tuple[Tree, list[str], bytes]:
     """Parse a policy file into a syntax tree using tree sitter.
 
-    This function is used by PolicyFile constructor, in most cases it will be
-    to call Policyfile(filename) instead of this function."""
+    This function is used by PolicyFile constructor, in most cases it is better
+    to call PolicyFile(filename) instead of this function."""
     assert os.path.isfile(filename)
     PY_LANGUAGE = Language(tscfengine.language())
     parser = Parser(PY_LANGUAGE)

src/cfengine_cli/main.py

@@ -59,7 +59,7 @@ def _get_arg_parser():
         default="yes",
         help="Strict mode. Default=yes, checks for undefined promise types, bundles, bodies, functions",
     )
-    lnt.add_argument("files", nargs="*", help="Files to format")
+    lnt.add_argument("files", nargs="*", help="Files to lint")
     subp.add_parser(
         "report",
         help="Run the agent and hub commands necessary to get new reporting data",
@@ -101,17 +101,17 @@ def _get_arg_parser():
 
     parser.add_argument(
         "--omit-download",
-        help="Use existing masterfiles instead of downloading in 'cfbs generate-release-information'",
+        help="Use existing masterfiles instead of downloading in 'cfengine dev generate-release-information'",
         action="store_true",
     )
     parser.add_argument(
         "--check-against-git",
-        help="Check whether masterfiles from cfengine.com and github.com match in 'cfbs generate-release-information'",
+        help="Check whether masterfiles from cfengine.com and github.com match in 'cfengine dev generate-release-information'",
         action="store_true",
     )
     parser.add_argument(
         "--from",
-        help="Specify minimum version in 'cfbs generate-release-information'",
+        help="Specify minimum version in 'cfengine dev generate-release-information'",
         dest="minimum_version",
     )
 

src/cfengine_cli/profile.py

@@ -106,7 +106,7 @@ def generate_callstack(data, stack_path):
         "Successfully generated callstack at '{}'".format(os.path.abspath(stack_path))
     )
     print(
-        "Run './flamgraph {} > flamegraph.svg' to generate the flamegraph".format(
+        "Run './flamegraph {} > flamegraph.svg' to generate the flamegraph".format(
             stack_path
         )
     )