From 10cb8114c67314f67b3000e1c58088561bda1a83 Mon Sep 17 00:00:00 2001 From: Ole Herman Schumacher Elgesem Date: Thu, 17 Jul 2025 01:11:48 +0200 Subject: [PATCH 1/2] Added some docstrings and TODOs Signed-off-by: Ole Herman Schumacher Elgesem --- cfbs/cfbs_config.py | 15 +++++++++++++++ cfbs/cfbs_json.py | 12 ++++++++++++ cfbs/commands.py | 39 +++++++++++++++++++++++++++++++++++++++ cfbs/validate.py | 11 +++++++++++ 4 files changed, 77 insertions(+) diff --git a/cfbs/cfbs_config.py b/cfbs/cfbs_config.py index e7ac18aa..01d15183 100644 --- a/cfbs/cfbs_config.py +++ b/cfbs/cfbs_config.py @@ -1,4 +1,19 @@ #!/usr/bin/env python3 +"""cfbs_config.py - Logic for manipulating a project / cfbs.json config file. + +TODOs: +- A lot of the code which is currently inside some long commands in commands.py + should be moved here. +- CFBSConfig.add() logic needs a good refactoring. It has a lot more code than + necessary duplicated for slightly different cases (URL vs index vs provides + vs dependencies). It should be rewritten / unified, and split up into a few + discrete steps: + 1. Collect modules to add (and filter) + 2. Validate modules and abort if the new modules fail validation + 3. Add modules and make commits +""" + + import os import copy import glob diff --git a/cfbs/cfbs_json.py b/cfbs/cfbs_json.py index 26f32e14..9bff7c0f 100644 --- a/cfbs/cfbs_json.py +++ b/cfbs/cfbs_json.py @@ -1,3 +1,15 @@ +"""cfbs_json.py - Simple read-only abstraction of a cfbs.json file + +Don't put code which makes changes to cfbs.json here, that should +go in cfbs_config.py. + +TODOs: +1. Code related to validation should be moved to validate.py. + For example warn_about_unknown_keys(). Validation code + should just take "raw data" (JSON dicts) and not be coupled + with cfbs_json.py nor cfbs_config.py. +""" + from collections import OrderedDict from copy import deepcopy import logging as log diff --git a/cfbs/commands.py b/cfbs/commands.py index e2f45643..154c050f 100644 --- a/cfbs/commands.py +++ b/cfbs/commands.py @@ -1,6 +1,45 @@ """ +commands.py - Entry points for each cfbs command. + +Each cfbs command has a corresponding function named _command. Functions ending in "_command" are dynamically included in the list of commands in main.py for -h/--help/help. + +At a high level, each command should do something like this: +1. Open the cfbs.json file using CFBSConfig.get_instance(). +2. Validate arguments and cfbs.json data before we start editing. +3. Perform the necessary operations using the CFBSConfig methods. +4. Validate cfbs.json again after editing. +5. Save changes and make commits. +6. Return an appropriate exit code. + 0 for success, 1 for failure. + Raising exceptions is also an option and often preferred, see main.py for exception handling code. + +Note that most of the business logic (step 2) should be implemented in CFBSConfig. + +The command function (in this file) should have parameters which +map closely to the command line arguments. + +For example, cfbs status does not take any arguments, so the signature should be: + +def status_command() -> int: + +On the other hand, cfbs search takes a list of search terms: + +cfbs search + +So, the appropriate signature is: + +def search_command(terms: List[str]) -> int: + +Todos: +1. Some of these functions are getting too long, business logic should be moved into + CFBSConfig in cfbs_config.py. Commands should generally not call other commands, + instead they should call the correct CFBSConfig method(s). +2. Decorators, like the git ones, should not change the parameters nor types of + the functions they decorate. This makes them much harder to read, and + type checkers don't understand it either. It applies to both types and values of + parameters and returns. """ import os diff --git a/cfbs/validate.py b/cfbs/validate.py index dbde1655..0f9a4d54 100644 --- a/cfbs/validate.py +++ b/cfbs/validate.py @@ -17,6 +17,17 @@ Thus, for example, the common parts needed by both build.py and validate.py, should be in utils.py or validate.py, not in build.py. + +TODOs: +1. Although we don't import anything from cfbs_config.py + nor cfbs_json.py, we still depend on them by calling + certain methods on the config object, such as + config.warn_about_unknown_keys() and config.find_module(). + We should decouple this so that validate.py is more pure + and can be called and tested with just simple JSON data + / dicts. This should make it easer to write unit tests, + use stricter type checking, and maintain all the validation + code in one place. """ import logging as log From 7303a736ca1918cbecbf5b8920e6d60cef8cb4b6 Mon Sep 17 00:00:00 2001 From: Ole Herman Schumacher Elgesem Date: Thu, 17 Jul 2025 14:35:15 +0200 Subject: [PATCH 2/2] Made cfbs add reject invalid modules Signed-off-by: Ole Herman Schumacher Elgesem --- cfbs/cfbs_config.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/cfbs/cfbs_config.py b/cfbs/cfbs_config.py index 01d15183..f5e46dc1 100644 --- a/cfbs/cfbs_config.py +++ b/cfbs/cfbs_config.py @@ -38,6 +38,7 @@ from cfbs.cfbs_json import CFBSJson from cfbs.module import Module, is_module_added_manually from cfbs.prompts import prompt_user, YES_NO_CHOICES +from cfbs.validate import validate_single_module # Legacy; do not use. Use the 'Result' namedtuple instead. @@ -348,6 +349,11 @@ def _add_without_dependencies(self, modules): del module["subdirectory"] if self.index.custom_index is not None: module["index"] = self.index.custom_index + # TODO: This validation could probably be done in a better place, + # after we refactor the add logic: + validate_single_module( + context="build", name=name, module=module, config=None, local_check=True + ) self["build"].append(module) self._handle_local_module(module)