From d5d2c1d703914e04f3dfde983edffebc15c3d523 Mon Sep 17 00:00:00 2001 From: Christian Herdtweck Date: Fri, 1 Apr 2022 16:41:42 +0200 Subject: [PATCH] Review: a few fixes functional changes: * get to run on python < 3.8 * add missing args for shadowed functions startswith/endswith * some more non-functional changes: * add some doc strings (mostly arnied_api) * wrap very long lines * add some whitespace to make linter happier * fix a few typos * remove unnecessary "object" super class * fix api documentation * add diagram that Samir had created to api doc --- .gitignore | 5 +++- doc/cnfvar-api-ascii-art.rst | 30 ++++++++++++++++++++++++ doc/conf.py | 12 +++++----- doc/index.rst | 2 +- src/arnied_api.py | 51 ++++++++++++++++++++++++++++++++++------- src/cnfvar/__init__.py | 2 +- src/cnfvar/binary.py | 20 +++++++++------- src/cnfvar/model.py | 34 ++++++++++++++------------- src/cnfvar/store.py | 45 ++++++++++++++++++++---------------- src/text_helpers.py | 2 +- test/cnfvar/test_store.py | 6 +++- 11 files changed, 143 insertions(+), 66 deletions(-) create mode 100644 doc/cnfvar-api-ascii-art.rst diff --git a/.gitignore b/.gitignore index 3d19e45..b8336d5 100644 --- a/.gitignore +++ b/.gitignore @@ -6,7 +6,10 @@ build/ dist/ MANIFEST _build/ -doc/ +doc/_build +doc/src.rst +doc/src.cnfline.rst +doc/src.cnfvar.rst # PyDev .project diff --git a/doc/cnfvar-api-ascii-art.rst b/doc/cnfvar-api-ascii-art.rst new file mode 100644 index 0000000..9f49bb7 --- /dev/null +++ b/doc/cnfvar-api-ascii-art.rst @@ -0,0 +1,30 @@ +Overview Diagram of cnfvar package +================================== + +:: + + frontend . CRUD layer . backend drivers . backend + | | | + | | | .-,( ),-. + o | | | .-( )-. + /|\ | .----------. | .---------->( varlink API ) + / \ |/| CnfStore | | | | '-( ).-' + / '----------' | | | '-.( ).-' + /| | | .-------------. | | + .----------' | | | | varlink API | | | + | CnfList | | '---------------->| interface | | | _.-----._ + '--.------. | | '-------------' | | .- -. + |.-----.\ | | | '->|-_ _-| + || Cnf | \| .----------------. | | | ~-----~ | + |'-----' \ | BinaryCnfStore | | | .->| cnfvar | + |.-----. |'-----------------' | | | `._backend_.' + || Cnf | | | | .------------. | | "-----" + |'-----' | | | | subprocess | | | + |.-----. | '-------------->| wrappers | | _______ + '| Cnf | | | '------------' | |get_cnf| + '-----' | | | | |set_cnf| + | | '------------> |_______| + | | | /:::::::/ + | | | + ' ' ' + diff --git a/doc/conf.py b/doc/conf.py index 97ef19d..ec74d6f 100644 --- a/doc/conf.py +++ b/doc/conf.py @@ -55,7 +55,7 @@ master_doc = 'index' # General information about the project. project = 'pyi2ncommon' -copyright = '2016-2018, Intra2net AG ' +copyright = 'Intra2net AG ' author = 'Intra2net AG' # The version info for the project you're documenting, acts as replacement for @@ -63,9 +63,9 @@ author = 'Intra2net AG' # built documents. # # The short X.Y version. -version = '0' +version = '1.6' # The full version, including alpha/beta/rc tags. -release = '1' +release = '7' # The language for content autogenerated by Sphinx. Refer to documentation # for a list of supported languages. @@ -293,7 +293,7 @@ texinfo_documents = [ # Example configuration for intersphinx: refer to the Python standard library. -intersphinx_mapping = {'https://docs.python.org/3.3': None} +intersphinx_mapping = {'https://docs.python.org/3': None} # show doc of class AND of constructor autoclass_content = 'class' # 'both' not needed if special-members is used @@ -307,8 +307,8 @@ autodoc_member_order = 'bysource' def skip_class_members(app, what, name, obj, skip, options): exclude = name in ('__weakref__', '__dict__', '__module__', '__doc__') \ and what == 'class' - if exclude: - print('excluding {0}'.format(name)) + #if exclude: + # print('excluding {0}'.format(name)) return skip or exclude def setup(app): diff --git a/doc/index.rst b/doc/index.rst index 7c85760..ffd006d 100644 --- a/doc/index.rst +++ b/doc/index.rst @@ -15,7 +15,7 @@ Contents: license.rst about_docu.rst - template + cnfvar-api-ascii-art.rst Modules diff --git a/src/arnied_api.py b/src/arnied_api.py index 5d8b868..537375b 100644 --- a/src/arnied_api.py +++ b/src/arnied_api.py @@ -24,6 +24,12 @@ arnied_api: wrappers around the arnied varlink API. Featuring - Arnied: stateless class with methods as exposed in the varlink API. +For documentation of Exceptions, methods and their arguments, refer to arnied +source code (arnied/client/arnieclient.hxx). For compatibility, argument names +are the same here as they are there (defeating python naming rules). + +See package `cnfvar` for a higher-level interface to this functionality. + .. codeauthor:: Intra2net """ @@ -44,15 +50,16 @@ ARNIED_SOCKET = "/var/intranator/arnied-varlink.sock" @dataclass -class CnfVar(object): +class CnfVar: name: str instance: int data: str deleted: bool children: typing.List['CnfVar'] + @dataclass -class ChangeCnfVar(object): +class ChangeCnfVar: chg_nr: int name: str instance: int @@ -60,11 +67,13 @@ class ChangeCnfVar(object): result_type: int result_msg: str + @dataclass -class GetCnfQuery(object): +class GetCnfQuery: name: str instance: typing.Optional[int] = None + class ProgramStatus(Enum): Running = auto() Scheduled = auto() @@ -76,19 +85,24 @@ class ProgramStatus(Enum): class IsQueueActiveRet(SimpleNamespace): active: bool + class IsScheduledOrRunningRet(SimpleNamespace): status: ProgramStatus timestamp: typing.Optional[int] + class GetCnfRet(SimpleNamespace): vars: typing.List[CnfVar] + class SetCnfRet(SimpleNamespace): change_numbers: typing.List[int] + class SetCommitCnf(SimpleNamespace): results: typing.List[ChangeCnfVar] + class CommitCnfRet(SimpleNamespace): results: typing.List[ChangeCnfVar] @@ -100,20 +114,24 @@ class InternalBridgeError(Exception): def __init__(self): super().__init__("An error occurred (InternalBridgeError)") + class EmptyInputError(Exception): def __init__(self): super().__init__("An error occurred (EmptyInputError)") + class BadCharError(Exception): def __init__(self, results: str) -> None: super().__init__(f"[BadCharError] Error in the arnied API (results={results})") self.results = results + class ChildError(Exception): def __init__(self, results: str) -> None: super().__init__(f"[ChildError] Error in the arnied API (results={results})") self.results = results + class CnfCommitError(Exception): def __init__(self, results: typing.List[ChangeCnfVar]) -> None: self.results = [] @@ -124,37 +142,50 @@ class CnfCommitError(Exception): r = ChangeCnfVar(**r) self.results.append(r) msgs.append(f"{r.name},{r.instance}: \"{r.data}\": {r.result_msg}") - super().__init__("Error commiting cnfvars:\n" + "\n".join(msgs)) + super().__init__("Error committing cnfvars:\n" + "\n".join(msgs)) + class NotFoundError(Exception): def __init__(self, results: str) -> None: super().__init__(f"[NotFoundError] Error in the arnied API (results={results})") self.results = results + class SchedulerProgError(Exception): def __init__(self): super().__init__("An error occurred (SchedulerProgError)") + class ShowerrVarDataErr(Exception): def __init__(self, msg_id: int) -> None: super().__init__(f"[ShowerrVarDataErr] Error in the arnied API (msg_id={msg_id})") self.msg_id = msg_id + class ShowerrVarMissing(Exception): def __init__(self, params_count: int) -> None: - super().__init__(f"[ShowerrVarMissing] Error in the arnied API (params_count={params_count})") + super().__init__( + f"[ShowerrVarMissing] Error in the arnied API (params_count={params_count})") self.params_count = params_count + class ProviderNotFound(Exception): def __init__(self, provider_id: int) -> None: - super().__init__(f"[ProviderNotFound] Not found a provider with ID `{provider_id})`") + super().__init__( + f"[ProviderNotFound] Could not find provider (provider_id={provider_id})") self.provider_id = provider_id # Arnied varlink proxy -class Arnied(object): +class Arnied: + """ + Expose methods of the arnied varlink interface in python. + + As described in module doc, documentation of exceptions, methods, and + their arguments can be found in arnied source code. + """ VARLINK_ADDRESS = f"unix:{ARNIED_SOCKET}" @classmethod @@ -238,12 +269,14 @@ class Arnied(object): return conn.SetCnf(vars) @classmethod - def set_commit_cnf(cls, vars: typing.List[CnfVar], username: typing.Optional[str], nogenerate: bool, fix_commit: bool) -> SetCommitCnf: + def set_commit_cnf(cls, vars: typing.List[CnfVar], username: typing.Optional[str], + nogenerate: bool, fix_commit: bool) -> SetCommitCnf: with cls.new_connection() as conn: return conn.SetCommitCnf(vars, username, nogenerate, fix_commit) @classmethod - def commit_cnf(cls, change_numbers: typing.List[int], username: typing.Optional[str], nogenerate: bool, fix_commit: bool) -> CommitCnfRet: + def commit_cnf(cls, change_numbers: typing.List[int], username: typing.Optional[str], + nogenerate: bool, fix_commit: bool) -> CommitCnfRet: with cls.new_connection() as conn: return conn.CommitCnf(change_numbers, username, nogenerate, fix_commit) diff --git a/src/cnfvar/__init__.py b/src/cnfvar/__init__.py index dd9a20e..3d60a25 100644 --- a/src/cnfvar/__init__.py +++ b/src/cnfvar/__init__.py @@ -2,5 +2,5 @@ from .model import Cnf, CnfList from .binary import CnfBinary from .store import CnfStore, BinaryCnfStore, CommitException -__all__ = ["Cnf, CnfList, CnfBinary", "CnfStore", +__all__ = ["Cnf", "CnfList", "CnfBinary", "CnfStore", "BinaryCnfStore", "CommitException"] diff --git a/src/cnfvar/binary.py b/src/cnfvar/binary.py index 3d71c19..63311cc 100644 --- a/src/cnfvar/binary.py +++ b/src/cnfvar/binary.py @@ -21,13 +21,15 @@ """ binary: wrappers around binaries that work with the CNF store. -Featuring -- CnfBinary: stateless class that can be used to invoke the different binaries -in a Python-friendly way. +Featuring: + - CnfBinary: stateless class that can be used to invoke the different binaries + in a Python-friendly way. .. note:: It is written as a class on purpose for it to be easily extended to -support invoking non-local binaries, but methods are all class methods since -the class is stateless. + support invoking non-local binaries, but methods are all class methods since + the class is stateless. + +.. seealso:: Overview Diagram linked to from doc main page .. codeauthor:: Intra2net """ @@ -38,7 +40,7 @@ import shlex import html import re -log = logging.getLogger("pyi2ncommon.cnfvar.store") +log = logging.getLogger("pyi2ncommon.cnfvar.binary") #: default get_cnf binary BIN_GET_CNF = "/usr/intranator/bin/get_cnf" @@ -48,7 +50,7 @@ BIN_SET_CNF = "/usr/intranator/bin/set_cnf" ENCODING = "latin1" -class CnfBinary(object): +class CnfBinary: """Provide wrappers around the multiple binaries to handle the CNF store.""" @classmethod @@ -93,7 +95,7 @@ class CnfBinary(object): :rtype: str .. note:: being a wrapper, this function does not do anything extra - like checking if arnied is running or waiting for generate. + like checking if arnied is running or waiting for generate. """ if name is None and instance is not None: raise ValueError("cannot pass `instance` without a `name`") @@ -137,7 +139,7 @@ class CnfBinary(object): :raises: :py:class:`SetCnfException` in case the binary errors out .. note:: being a wrapper, this function does not do anything extra - like checking if arnied is running or waiting for generate. + like checking if arnied is running or waiting for generate. """ if input_str is None and input_file is None: raise ValueError("Need to pass either a string or a file") diff --git a/src/cnfvar/model.py b/src/cnfvar/model.py index 3c3fbbf..242a9d3 100644 --- a/src/cnfvar/model.py +++ b/src/cnfvar/model.py @@ -21,14 +21,15 @@ """ model: Cnf classes, collection of Cnf classes and multiple filtering methods. -Featuring -- Cnf: class representing a CNF variabe - -- CnfList: a collection of `Cnf` instances +Featuring: + - Cnf: class representing a CNF variable + - CnfList: a collection of `Cnf` instances The classes above inherit from their base types with added mixins which extend them with extra functionality. +.. seealso:: Overview Diagram linked to from doc main page + .. codeauthor:: Intra2net """ @@ -64,11 +65,11 @@ class CnfName(str): def __contains__(self, name): return name.lower() in self.lower() - def startswith(self, prefix): - return self.lower().startswith(prefix.lower()) + def startswith(self, prefix, *args, **kwargs): + return self.lower().startswith(prefix.lower(), *args, **kwargs) - def endswith(self, prefix): - return self.lower().endswith(prefix.lower()) + def endswith(self, prefix, *args, **kwargs): + return self.lower().endswith(prefix.lower(), *args, **kwargs) ############################################################################### @@ -104,6 +105,7 @@ class BaseCnfList(list): else: iter_ = [] super().__init__(iter_) + self._renumber_counter = None # initialized and used in renumber if renumber: self.renumber() @@ -112,11 +114,11 @@ class BaseCnfList(list): # NOTE: we don't keep track of operations that change the list as this # would require us to reimplement most of the methods. At least for now # this method should be called again when serializing. - self._counter = 0 + self._renumber_counter = 0 def renumber_fn(cnf): - self._counter += 1 - cnf.lineno = self._counter + self._renumber_counter += 1 + cnf.lineno = self._renumber_counter self.for_each_all(renumber_fn) @@ -126,7 +128,7 @@ class BaseCnfList(list): :param where_filter: predicate to apply against CNFs :type where_filter: function accepting a CNF and returning a boolean - :returns: a instance of this class with filtered members + :returns: an instance of this class with filtered members :rtype: :py:class:`CnfList` """ return CnfList(c for c in self if where_filter(c)) @@ -137,7 +139,7 @@ class BaseCnfList(list): :param where_filter: predicate to apply against children :type where_filter: function accepting a CNF and returning a boolean - :returns: a instance of this class with filtered members + :returns: an instance of this class with filtered members :rtype: :py:class:`CnfList` """ def upper_filter(cnf): @@ -530,7 +532,7 @@ class CnfListSerializationMixin(BaseCnfList): d["children"] = [_to_dict(c) for c in cnf.children] return d if renumber: - renumber = True + self.renumber() json_list = [_to_dict(c) for c in self] return json.dumps({"cnf": json_list}) @@ -813,7 +815,7 @@ class CnfShortcutsMixin(BaseCnf): """ cnf = self.children.first_with_name(name, default=None) if cnf is None: - cnf = self.add_child(name, "1") + self.add_child(name, "1") else: cnf.enable() @@ -827,7 +829,7 @@ class CnfShortcutsMixin(BaseCnf): """ cnf = self.children.first_with_name(name, default=None) if cnf is None: - cnf = self.add_child(name, "0") + self.add_child(name, "0") else: cnf.disable() diff --git a/src/cnfvar/store.py b/src/cnfvar/store.py index 9872c16..0bfeb26 100644 --- a/src/cnfvar/store.py +++ b/src/cnfvar/store.py @@ -19,13 +19,14 @@ # Copyright (c) 2016-2022 Intra2net AG """ -store: implementations of CNF stores using varlink and *et_cnf binaries. +store: implementations of CNF stores using varlink and `*et_cnf` binaries. -Featuring -- CnfStore: the main store class that is implemented using the varlink API +Featuring: + - CnfStore: the main store class that is implemented using the varlink API + - BinaryCnfStore: alternative store class implementation using the `set_cnf` + and `get_cnf` binaries -- BinaryCnfStore: alternative store class implementation using the set_ and -get_cnf binaries +.. seealso:: Overview Diagram linked to from doc main page .. codeauthor:: Intra2net """ @@ -89,7 +90,7 @@ class CnfStore: :param bool fix_problems: whether to automatically fix errors in the vars .. note:: you can mix variables to insert and variables to update - in the same list as the system should handle it nicely + in the same list as the system should handle it nicely Example:: store = CnfStore() @@ -241,7 +242,7 @@ class CnfStore: class BinaryCnfStore(CnfStore): - """Implementation of the CNF store that uses get_ and set_cnf.""" + """Implementation of the CNF store that uses `get_cnf` and `set_cnf`.""" #: how much to wait for arnied to report running ARNIED_TIMEOUT = 30 @@ -272,7 +273,7 @@ class BinaryCnfStore(CnfStore): """ log.debug("Querying BinaryCnfStore with name=%s and instance=%s", name, instance) - output = self._driver.get_cnf(name, instance) + output = self._driver.get_cnf(name, instance=instance) if len(output) == 0: # otherwise cnfvar raises a generic Malformed exception @@ -289,7 +290,7 @@ class BinaryCnfStore(CnfStore): :param bool fix_problems: whether to automatically fix errors in the vars .. note:: you can mix variables to insert and variables to update - in the same list as the system should handle it nicely + in the same list as the system should handle it nicely Example:: store = CnfStore() @@ -301,7 +302,7 @@ class BinaryCnfStore(CnfStore): cnf = self._cnf_or_list(cnf, operation="commit") self._autofix_instances(cnf) - # set_cnf is demaning on lineno's + # set_cnf is demanding on lineno's cnf.renumber() log.debug("Committing variables via binaries:\n%s", cnf) @@ -330,7 +331,7 @@ class BinaryCnfStore(CnfStore): if any((c.parent is not None for c in cnf)): raise RuntimeError("Calling delete is only supported on top-level CNF variables") - # set_cnf is demaning on lineno's + # set_cnf is demanding on lineno's cnf.renumber() log.debug("Deleting variables via binaries:\n%s", cnf) @@ -353,6 +354,18 @@ class BinaryCnfStore(CnfStore): return fn(*args, **kwargs) +#: pattern for more verbose error message for :py:class:`CommitException` +COMMIT_EXCEPTION_MESSAGE = """\ +Error committing CNF variables! +---------------------------- +Input: +{cnfvars} +---------------------------- +Error: +{msg} +""" + + class CommitException(Exception): """Custom exception for commit errors.""" @@ -364,13 +377,5 @@ class CommitException(Exception): :type cnfvars: CnfList :param str msg: error message """ - self.message = f"""\ -Error committing CNF variables! ----------------------------- -Input: -{cnfvars} ----------------------------- -Error: -{msg} -""" super().__init__(msg) + self.message = COMMIT_EXCEPTION_MESSAGE.format(cnfvars=cnfvars, msg=msg) diff --git a/src/text_helpers.py b/src/text_helpers.py index cd33cf0..45680a8 100644 --- a/src/text_helpers.py +++ b/src/text_helpers.py @@ -242,7 +242,7 @@ def print(text, *args, **kwargs): # pylint: disable=redefined-builtin """ like the builtin print function but also accepts color args If any arg of :py:func:`colored` is given in `kwargs`, will run text with - color-args through that function. Runs built-in :py:builtin:`print` + color-args through that function. Runs built-in :py:func:`print` function with result and other args. ...todo:: color all args, not just first diff --git a/test/cnfvar/test_store.py b/test/cnfvar/test_store.py index e234c4d..b5612d3 100644 --- a/test/cnfvar/test_store.py +++ b/test/cnfvar/test_store.py @@ -57,7 +57,8 @@ class TestStore(unittest.TestCase): patch.object(store, "_wait_for_generate"): self._query_and_update(store) - cnf_api_input = set_cnf_mock.call_args.kwargs["vars"] + args, kwargs = set_cnf_mock.call_args + cnf_api_input = kwargs["vars"] cnf_input = CnfList.from_api_structure(cnf_api_input) expected_input = dedent("""\ @@ -100,7 +101,8 @@ class TestStore(unittest.TestCase): patch.object(store, "_wait_for_generate"): store.delete(cnfvars) - cnf_api_input = set_cnf_mock.call_args.kwargs["vars"] + args, kwargs = set_cnf_mock.call_args + cnf_api_input = kwargs["vars"] cnf_input = CnfList.from_api_structure(cnf_api_input) expected_input = dedent("""\ -- 1.7.1