Minor code quality improvements

JSON.md

@@ -224,6 +224,7 @@ These are copies of the module directories, where it's more "safe" to do things
 
 The build steps below manipulate the temporary files in the steps directories and write results to the output policy set, in `out/masterfiles`.
 Unless otherwise noted, all steps are run inside the module's folder (`out/steps/...`) with sources / file paths relative to that folder, and targets / destinations mentioned below are relative to the output policy set (`out/masterfiles`, which in the end will be deployed as `/var/cfengine/masterfiles`).
+In `cfbs.json`'s `"steps"`, the build step name must be separated from the rest of the build step by a regular space.
 
 - `copy <source> <destination>`
   - Copy a single file or a directory recursively.
@@ -450,11 +451,9 @@ cfbs init && cfbs add https://github.com/cfengine/some-repo
 
 ## Modules with input
 
-Some modules allow for users to add module input by responding to questions
-expressed under the `"input"` attribute in `cfbs.json`. User input can be added
-using the `cfbs input <module-name>` command, which stores responses in
-`./<module-name>/input.json`. These responses are translated into augments which
-will be added to `./out/masterfiles/def.json` during `cfbs build`.
+Some modules allow for users to add module input by responding to questions expressed under the `"input"` attribute in `cfbs.json`.
+User input can be added using the `cfbs input <module-name>` command, which stores responses in `./<module-name>/input.json`.
+These responses are translated into augments which will be added to `./out/masterfiles/def.json` during `cfbs build`.
 
 ### Create single file example
 
@@ -490,10 +489,8 @@ The `"input"` attribute takes a list of input definitions as illustrated below.
 }
 ```
 
-From the example above, we can see that the `"input"` list contains one input
-definition. By running the command `cfbs input create-single-file`, the input
-definition will be copied into `./create-single-file/input.json` along with the
-user responses.
+From the example above, we can see that the `"input"` list contains one input definition.
+By running the command `cfbs input create-single-file`, the input definition will be copied into `./create-single-file/input.json` along with the user responses.
 
 ```
 $ cfbs input create-single-file
@@ -511,10 +508,8 @@ $ cat ./create-single-file/input.json
 ]
 ```
 
-By running `cfbs build`, augments will be generated from
-`./create-single-file/input.json` and added to `./out/masterfiles/def.json`.
-Note that this is dependant on the `"input ./input.json def.json"` build step in
-`cfbs.json`.
+By running `cfbs build`, augments will be generated from `./create-single-file/input.json` and added to `./out/masterfiles/def.json`.
+Note that this is dependent on the `"input ./input.json def.json"` build step in `cfbs.json`.
 
 ```
 $ cfbs build
@@ -538,13 +533,11 @@ $ cat ./out/masterfiles/def.json
 }
 ```
 
-From the example above we can see our beloved `filename`-variable along with a
-class generated by the autorun dependency. Studying our variable closer, we can
-see that a namespace, bundle, and a comment, were automatically assigned some
-default values. I.e. `cfbs`, the module name canonified, and `Added by 'cfbs
-input'` respectivy. These defaults can easily be overridden using the
-`namespace`, `bundle`, and `comment` attributes in the variable definition. E.g.
-the following variable definition;
+From the example above we can see our beloved `filename`-variable along with a class generated by the autorun dependency.
+Studying our variable closer, we can see that a namespace, bundle, and a comment, were automatically assigned some default values.
+I.e. `cfbs`, the module name canonified, and `Added by 'cfbs input'` respectively.
+These defaults can easily be overridden using the `namespace`, `bundle`, and `comment` attributes in the variable definition.
+E.g. the following variable definition;
 
 ```json
       "input": [
@@ -575,9 +568,8 @@ would produce the following augment;
 
 ### Create a single file with content example
 
-A module that creates empty files is not too impressive on its own. Let us
-instead try to extend our previous example by having the module also ask for
-file contents.
+A module that creates empty files is not too impressive on its own.
+Let us instead try to extend our previous example by having the module also ask for file contents.
 
 ```json
 {
@@ -615,9 +607,8 @@ file contents.
 }
 ```
 
-As you can see from the example above, the extension would only require us to
-add another variable to the input definition. Let's have a look at the results
-from running `cfbs input` with our extension module.
+As you can see from the example above, the extension would only require us to add another variable to the input definition.
+Let's have a look at the results from running `cfbs input` with our extension module.
 
 ```
 $ cfbs input create-single-file-with-content
@@ -651,9 +642,9 @@ $ cat ./out/masterfiles/def.json
 
 ### Create multiple files example
 
-Sometimes we would like a module to support taking an arbritary number of
-inputs. This can be done using a variable definition of type list. Let's extend
-our first example from creating a single to multiple files.
+Sometimes we would like a module to support taking an arbritary number of inputs.
+This can be done using a variable definition of type list.
+Let's extend our first example from creating a single to multiple files.
 
 ```json
 {
@@ -702,8 +693,7 @@ What file should this module create? /tmp/create-multiple-files-2.txt
 Do you want to create another file? no
 ```
 
-The _*./create-multiple-files/input.json*_ file would look similar to the
-following JSON:
+The _*./create-multiple-files/input.json*_ file would look similar to the following JSON:
 
 ```json
 [
@@ -725,8 +715,7 @@ following JSON:
 ]
 ```
 
-And if we build our project we can expect something similar to the following
-output:
+And if we build our project we can expect something similar to the following output:
 
 ```
 $ cfbs build
@@ -755,8 +744,7 @@ $ cat ./out/masterfiles/def.json
 
 ### Create multiple files with content example
 
-As a final example, let's see how we can build a module that takes an arbritary
-number of filename and content pairs as input.
+As a final example, let's see how we can build a module that takes an arbitrary number of filename and content pairs as input.
 
 ```json
 {

cfbs/build.py

@@ -1,18 +1,17 @@
 import os
 import logging as log
+from typing import List, Tuple
 from cfbs.utils import (
     canonify,
     cp,
     deduplicate_def_json,
     find,
-    is_valid_arg_count,
     merge_json,
     mkdir,
     pad_right,
     read_json,
     rm,
     sh,
-    split_command,
     strip_left,
     touch,
     user_error,
@@ -73,8 +72,31 @@ def _generate_augment(module_name, input_data):
     return augment
 
 
+def split_build_step(command) -> Tuple[str, List[str]]:
+    terms = command.split(" ")
+    operation, args = terms[0], terms[1:]
+    return operation, args
+
+
+def step_has_valid_arg_count(args, expected):
+    actual = len(args)
+
+    if type(expected) is int:
+        if actual != expected:
+            return False
+
+    else:
+        # Only other option is a string of 1+, 2+ or similar:
+        assert type(expected) is str and expected.endswith("+")
+        expected = int(expected[0:-1])
+        if actual < expected:
+            return False
+
+    return True
+
+
 def _perform_build_step(module, step, max_length):
-    operation, args = split_command(step)
+    operation, args = split_build_step(step)
     source = module["_directory"]
     counter = module["_counter"]
     destination = "out/masterfiles"
@@ -245,7 +267,7 @@ def perform_build_steps(config) -> int:
     # mini-validation
     for module in config.get("build", []):
         for step in module["steps"]:
-            operation, args = split_command(step)
+            operation, args = split_build_step(step)
 
             if step.split() != [operation] + args:
                 user_error(
@@ -258,7 +280,7 @@ def perform_build_steps(config) -> int:
 
             expected = AVAILABLE_BUILD_STEPS[operation]
             actual = len(args)
-            if not is_valid_arg_count(args, expected):
+            if not step_has_valid_arg_count(args, expected):
                 if type(expected) is int:
                     user_error(
                         "The `%s` build step expects %d arguments, %d were given"

cfbs/utils.py

@@ -5,8 +5,6 @@
 import copy
 import subprocess
 import hashlib
-import logging as log
-from typing import List, Tuple
 import urllib
 import urllib.request  # needed on some platforms
 from collections import OrderedDict
@@ -79,35 +77,12 @@ def cp(src, dst):
     sh("rsync -r %s/ %s" % (src, dst))
 
 
-def pad_left(s, n) -> int:
-    return s if len(s) >= n else " " * (n - len(s)) + s
+def pad_left(s, n):
+    return s.rjust(n)
 
 
-def pad_right(s, n) -> int:
-    return s if len(s) >= n else s + " " * (n - len(s))
-
-
-def split_command(command) -> Tuple[str, List[str]]:
-    terms = command.split(" ")
-    operation, args = terms[0], terms[1:]
-    return operation, args
-
-
-def is_valid_arg_count(args, expected):
-    actual = len(args)
-
-    if type(expected) is int:
-        if actual != expected:
-            return False
-
-    else:
-        # Only other option is a string of 1+, 2+ or similar:
-        assert type(expected) is str and expected.endswith("+")
-        expected = int(expected[0:-1])
-        if actual < expected:
-            return False
-
-    return True
+def pad_right(s, n):
+    return s.ljust(n)
 
 
 def user_error(msg: str):
@@ -140,12 +115,14 @@ def item_index(iterable, item, extra_at_end=True):
 
 
 def strip_right(string, ending):
+    # can be replaced with str.removesuffix from Python 3.9 onwards
     if not string.endswith(ending):
         return string
     return string[0 : -len(ending)]
 
 
 def strip_left(string, beginning):
+    # can be replaced with str.removeprefix from Python 3.9 onwards
     if not string.startswith(beginning):
         return string
     return string[len(beginning) :]
@@ -166,7 +143,7 @@ def save_file(path, data):
         f.write(data)
 
 
-def read_json(path):
+def read_json(path) -> OrderedDict:
     try:
         with open(path, "r") as f:
             return json.loads(f.read(), object_pairs_hook=OrderedDict)
@@ -175,7 +152,7 @@ def read_json(path):
     except NotADirectoryError:
         return None
     except json.decoder.JSONDecodeError as ex:
-        print("Error reading json file {} : {}".format(path, ex))
+        print("Error reading json file '{}': {}".format(path, ex))
         sys.exit(1)
 
 
@@ -275,15 +252,24 @@ def is_cfbs_repo() -> bool:
 
 
 def immediate_subdirectories(path):
-    return [f.name for f in os.scandir(path) if f.is_dir()]
+    l = [f.name for f in os.scandir(path) if f.is_dir()]
+
+    # `os.scandir` returns the entries in arbitrary order, so sort for determinism
+    l = sorted(l)
+
+    return l
 
 
 def immediate_files(path):
-    return [f.name for f in os.scandir(path) if not f.is_dir()]
+    l = [f.name for f in os.scandir(path) if not f.is_dir()]
+
+    # `os.scandir` returns the entries in arbitrary order, so sort for determinism
+    l = sorted(l)
+
+    return l
 
 
 def path_append(dir, subdir):
-    dir = os.path.abspath(os.path.expanduser(dir))
     return dir if not subdir else os.path.join(dir, subdir)
 
 
@@ -299,7 +285,10 @@ def are_paths_equal(path_a, path_b) -> bool:
 
 
 def cfengine_dir(subdir=None):
-    return path_append("~/.cfengine/", subdir)
+    CFENGINE_DIR = "~/.cfengine/"
+    cfengine_dir_abspath = os.path.abspath(os.path.expanduser(CFENGINE_DIR))
+
+    return path_append(cfengine_dir_abspath, subdir)
 
 
 def cfbs_dir(append=None) -> str:

cfbs/validate.py

@@ -1,13 +1,15 @@
 import argparse
-import json
 import sys
 import re
 from collections import OrderedDict
 
-from cfbs.utils import is_valid_arg_count, is_a_commit_hash, split_command, user_error
+from cfbs.utils import (
+    is_a_commit_hash,
+    user_error,
+)
 from cfbs.pretty import TOP_LEVEL_KEYS, MODULE_KEYS
 from cfbs.cfbs_config import CFBSConfig
-from cfbs.build import AVAILABLE_BUILD_STEPS
+from cfbs.build import AVAILABLE_BUILD_STEPS, step_has_valid_arg_count, split_build_step
 
 
 class CFBSValidationError(Exception):
@@ -266,7 +268,7 @@ def validate_steps(name, module):
                 raise CFBSValidationError(
                     name, '"steps" must be a list of non-empty / non-whitespace strings'
                 )
-            operation, args = split_command(step)
+            operation, args = split_build_step(step)
             if not operation in AVAILABLE_BUILD_STEPS:
                 x = ", ".join(AVAILABLE_BUILD_STEPS)
                 raise CFBSValidationError(
@@ -276,7 +278,7 @@ def validate_steps(name, module):
                 )
             expected = AVAILABLE_BUILD_STEPS[operation]
             actual = len(args)
-            if not is_valid_arg_count(args, expected):
+            if not step_has_valid_arg_count(args, expected):
                 if type(expected) is int:
                     raise CFBSValidationError(
                         name,

tests/test_utils.py

@@ -230,32 +230,19 @@ def test_immediate_subdirectories():
     path = "tests/sample/sample_dir"
     expected = ["sample_subdir_A", "sample_subdir_B"]
 
-    actual = immediate_subdirectories(path)
-    # `immediate_subdirectories` currently returns the entries in arbitrary order
-    actual = sorted(actual)
-
-    assert actual == expected
+    assert immediate_subdirectories(path) == expected
 
 
 def test_immediate_files():
     path = "tests/sample/sample_dir"
     expected = ["sample_file_1.txt", "sample_file_2.txt"]
 
-    actual = immediate_files(path)
-    # `immediate_files` currently returns the entries in arbitrary order
-    actual = sorted(actual)
-
-    assert actual == expected
+    assert immediate_files(path) == expected
 
 
 def test_path_append():
     path = "tests/sample/sample_dir"
 
-    # `path_append` is currently coupled with the below code
-    import os
-
-    path = os.path.abspath(os.path.expanduser(path))
-
     assert path_append(path, "abc") == path + "/abc"
     assert path_append(path, None) == path