From 022c5151e88dfb268fa2ba65f505e579ffccf781 Mon Sep 17 00:00:00 2001 From: James Garner Date: Tue, 15 Oct 2024 15:51:02 +1300 Subject: [PATCH 1/4] style: use pythonic style Including: - raising ValueError instead of Exception - using `isinstance` instead of `type(...) is ...` - multi-line continuation style --- juju/constraints.py | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/juju/constraints.py b/juju/constraints.py index 3315feadc..e57980d56 100644 --- a/juju/constraints.py +++ b/juju/constraints.py @@ -52,7 +52,8 @@ "spaces", "virt_type", "zones", - "allocate_public_ip"] + "allocate_public_ip", +] LIST_KEYS = {'tags', 'spaces', 'zones'} @@ -69,18 +70,21 @@ def parse(constraints): if not constraints: return None - if type(constraints) is dict: + if isinstance(constraints, dict): # Fowards compatibilty: already parsed return constraints normalized_constraints = {} for s in constraints.split(" "): if "=" not in s: - raise Exception("malformed constraint %s" % s) + raise ValueError("malformed constraint %s" % s) k, v = s.split("=") - normalized_constraints[normalize_key(k)] = normalize_list_value(v) if\ - k in LIST_KEYS else normalize_value(v) + normalized_constraints[normalize_key(k)] = ( + normalize_list_value(v) + if k in LIST_KEYS + else normalize_value(v) + ) return normalized_constraints @@ -95,7 +99,7 @@ def normalize_key(orig_key): key = SNAKE2.sub(r'\1_\2', key).lower() if key not in SUPPORTED_KEYS: - raise Exception("unknown constraint in %s" % orig_key) + raise ValueError("unknown constraint in %s" % orig_key) return key @@ -132,7 +136,7 @@ def parse_storage_constraint(constraint): pool = m.group('pool') if pool: if 'pool' in storage: - raise Exception("pool already specified") + raise ValueError("pool already specified") storage['pool'] = pool count = m.group('count') if count: @@ -152,7 +156,7 @@ def parse_storage_constraint(constraint): def parse_device_constraint(constraint): m = DEVICE.match(constraint) if m is None: - raise Exception("device constraint does not match") + raise ValueError("device constraint does not match") device = {} count = m.group('count') if count: From 3addc27f2a7b0a40f62fa0ea061ac5e8f2667b4c Mon Sep 17 00:00:00 2001 From: James Garner Date: Tue, 15 Oct 2024 15:56:09 +1300 Subject: [PATCH 2/4] style: add type annotations --- juju/constraints.py | 54 +++++++++++++++++++++++++++++++++++++-------- 1 file changed, 45 insertions(+), 9 deletions(-) diff --git a/juju/constraints.py b/juju/constraints.py index e57980d56..620120b95 100644 --- a/juju/constraints.py +++ b/juju/constraints.py @@ -19,6 +19,9 @@ # import re +from typing import Dict, List, Optional, TypedDict, Union +from typing_extensions import Required, NotRequired + # Matches on a string specifying memory size MEM = re.compile('^[1-9][0-9]*[MGTP]$') @@ -61,7 +64,28 @@ SNAKE2 = re.compile('([a-z0-9])([A-Z])') -def parse(constraints): +ParsedValue = Union[int, bool, str] + + +class ConstraintsDict(TypedDict, total=False): + allocate_public_ip: ParsedValue + arch: ParsedValue + container: ParsedValue + cores: ParsedValue + cpu_cores: ParsedValue + cpu_power: ParsedValue + instance_role: ParsedValue + instance_type: ParsedValue + mem: ParsedValue + root_disk: ParsedValue + root_dist_source: ParsedValue + spaces: List[ParsedValue] + tags: List[ParsedValue] + virt_type: ParsedValue + zones: List[ParsedValue] + + +def parse(constraints: Union[str, ConstraintsDict]) -> Optional[ConstraintsDict]: """ Constraints must be expressed as a string containing only spaces and key value pairs joined by an '='. @@ -74,7 +98,7 @@ def parse(constraints): # Fowards compatibilty: already parsed return constraints - normalized_constraints = {} + normalized_constraints: ConstraintsDict = {} for s in constraints.split(" "): if "=" not in s: raise ValueError("malformed constraint %s" % s) @@ -89,7 +113,7 @@ def parse(constraints): return normalized_constraints -def normalize_key(orig_key): +def normalize_key(orig_key: str) -> str: key = orig_key.strip() key = key.replace("-", "_") # Our _client lib wants "_" in place of "-" @@ -103,7 +127,7 @@ def normalize_key(orig_key): return key -def normalize_value(value): +def normalize_value(value: str) -> Union[int, bool, str]: value = value.strip() if MEM.match(value): @@ -121,7 +145,7 @@ def normalize_value(value): return value -def normalize_list_value(value): +def normalize_list_value(value: str) -> List[ParsedValue]: values = value.strip().split(',') return [normalize_value(value) for value in values] @@ -130,8 +154,14 @@ def normalize_list_value(value): '(?:(?:^|(?<=,))(?:|(?P[a-zA-Z]+[-?a-zA-Z0-9]*)|(?P-?[0-9]+)|(?:(?P-?[0-9]+(?:\\.[0-9]+)?)(?P[MGTPEZY])(?:i?B)?))(?:$|,))') -def parse_storage_constraint(constraint): - storage = {'count': 1} +class StorageConstraintDict(TypedDict): + count: Required[int] # >= 1 + pool: NotRequired[str] + size: NotRequired[int] + + +def parse_storage_constraint(constraint: str) -> StorageConstraintDict: + storage: StorageConstraintDict = {'count': 1} for m in STORAGE.finditer(constraint): pool = m.group('pool') if pool: @@ -153,11 +183,17 @@ def parse_storage_constraint(constraint): ATTR = re.compile(';?(?P[^=]+)=(?P[^;]+)') -def parse_device_constraint(constraint): +class DeviceConstraintDict(TypedDict): + count: Required[int] + type: Required[str] + attributes: NotRequired[Dict[str, str]] + + +def parse_device_constraint(constraint: str) -> DeviceConstraintDict: m = DEVICE.match(constraint) if m is None: raise ValueError("device constraint does not match") - device = {} + device: DeviceConstraintDict = {} count = m.group('count') if count: count = int(count) From d117fe41872ff13da0424a3dbdd54da1c2aec3ef Mon Sep 17 00:00:00 2001 From: James Garner Date: Tue, 15 Oct 2024 16:01:01 +1300 Subject: [PATCH 3/4] style: split complex regexes across multiple lines and add comments --- juju/constraints.py | 32 ++++++++++++++++++++++++++++++-- 1 file changed, 30 insertions(+), 2 deletions(-) diff --git a/juju/constraints.py b/juju/constraints.py index 620120b95..65f341f11 100644 --- a/juju/constraints.py +++ b/juju/constraints.py @@ -151,7 +151,21 @@ def normalize_list_value(value: str) -> List[ParsedValue]: STORAGE = re.compile( - '(?:(?:^|(?<=,))(?:|(?P[a-zA-Z]+[-?a-zA-Z0-9]*)|(?P-?[0-9]+)|(?:(?P-?[0-9]+(?:\\.[0-9]+)?)(?P[MGTPEZY])(?:i?B)?))(?:$|,))') + # original regex: + # '(?:(?:^|(?<=,))(?:|(?P[a-zA-Z]+[-?a-zA-Z0-9]*)|(?P-?[0-9]+)|(?:(?P-?[0-9]+(?:\\.[0-9]+)?)(?P[MGTPEZY])(?:i?B)?))(?:$|,))' + # with formatting and explanation -- note that this regex is used with re.finditer: + '(?:(?:^|(?<=,))' # start of string or previous match ends with ',' + '(?:' # match one of the following: + '|(?P[a-zA-Z]+[-?a-zA-Z0-9]*)' # pool: a sequence starting with a letter, ending with a letter or number, + # -- and including letters, numbers and hyphens (no more than one in a row) + '|(?P-?[0-9]+)' # count: an optional minus sign followed by one or more digits + '|(?:' # size (number) and size_exp (units): + '(?P-?[0-9]+(?:\\.[0-9]+)?)' # an optional minus sign followed by one or more digits, optionally with decimal point and more digits + '(?P[MGTPEZY])(?:i?B)?' # one of MGTPEZY, optionally followed by iB or B, for example 1M or 2.0MB or -3.3MiB + ')' + ')' + '(?:$|,))' # end of string or ',' +) class StorageConstraintDict(TypedDict): @@ -179,7 +193,21 @@ def parse_storage_constraint(constraint: str) -> StorageConstraintDict: DEVICE = re.compile( - '^(?P[0-9]+)?(?:^|,)(?P[^,]+)(?:$|,(?!$))(?P(?:[^=]+=[^;]+)+)*$') + # original regex: + # '^(?P[0-9]+)?(?:^|,)(?P[^,]+)(?:$|,(?!$))(?P(?:[^=]+=[^;]+)+)*$' + # with formatting and explanation: + '^' # start of string + '(?P[0-9]+)?' # count is 1+ digits, and is optional + '(?:^|,)' # match start of string or a comma + # -- so type can be at the start or comma separated from count + '(?P[^,]+)' # type is 1+ anything not a comma (including digits), and is required + '(?:$|,(?!$))' # match end of string | or a non-trailing comma + # -- so type can be at the end or followed by attrs + '(?P(?:[^=]+=[^;]+)+)*' # attrs is any number of semicolon separated key=value items + # -- value can have spare '=' inside, possible not intended + # -- attrs will be matched with ATTR.finditer afterwards in parse_device_constraint + '$' # end of string +) ATTR = re.compile(';?(?P[^=]+)=(?P[^;]+)') From c49e474ebf496b628e2015429ed87e0828f5984f Mon Sep 17 00:00:00 2001 From: James Garner Date: Tue, 15 Oct 2024 16:17:28 +1300 Subject: [PATCH 4/4] style: make flake8 happy about indentation of continuations --- juju/constraints.py | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/juju/constraints.py b/juju/constraints.py index 65f341f11..44ee5fbe1 100644 --- a/juju/constraints.py +++ b/juju/constraints.py @@ -154,17 +154,18 @@ def normalize_list_value(value: str) -> List[ParsedValue]: # original regex: # '(?:(?:^|(?<=,))(?:|(?P[a-zA-Z]+[-?a-zA-Z0-9]*)|(?P-?[0-9]+)|(?:(?P-?[0-9]+(?:\\.[0-9]+)?)(?P[MGTPEZY])(?:i?B)?))(?:$|,))' # with formatting and explanation -- note that this regex is used with re.finditer: - '(?:(?:^|(?<=,))' # start of string or previous match ends with ',' - '(?:' # match one of the following: - '|(?P[a-zA-Z]+[-?a-zA-Z0-9]*)' # pool: a sequence starting with a letter, ending with a letter or number, - # -- and including letters, numbers and hyphens (no more than one in a row) - '|(?P-?[0-9]+)' # count: an optional minus sign followed by one or more digits - '|(?:' # size (number) and size_exp (units): - '(?P-?[0-9]+(?:\\.[0-9]+)?)' # an optional minus sign followed by one or more digits, optionally with decimal point and more digits - '(?P[MGTPEZY])(?:i?B)?' # one of MGTPEZY, optionally followed by iB or B, for example 1M or 2.0MB or -3.3MiB - ')' + '(?:' + '(?:^|(?<=,))' # start of string or previous match ends with ',' + '(?:' # match one of the following: + '|(?P[a-zA-Z]+[-?a-zA-Z0-9]*)' # * pool: a sequence starting with a letter, ending with a letter or number, + # ------- and including letters, numbers and hyphens (no more than one in a row) + '|(?P-?[0-9]+)' # * count: an optional minus sign followed by one or more digits + '|(?:' # * size (number) and size_exp (units): + '(?P-?[0-9]+(?:\\.[0-9]+)?)' # -- * an optional minus sign followed by one or more digits, optionally with decimal point and more digits + '(?P[MGTPEZY])(?:i?B)?)' # -- * one of MGTPEZY, optionally followed by iB or B, for example 1M or 2.0MB or -3.3MiB + ')' + '(?:$|,)' # end of string or ',' ')' - '(?:$|,))' # end of string or ',' ) @@ -195,7 +196,7 @@ def parse_storage_constraint(constraint: str) -> StorageConstraintDict: DEVICE = re.compile( # original regex: # '^(?P[0-9]+)?(?:^|,)(?P[^,]+)(?:$|,(?!$))(?P(?:[^=]+=[^;]+)+)*$' - # with formatting and explanation: + # with formatting and explanation -- note this regex is used with re.match: '^' # start of string '(?P[0-9]+)?' # count is 1+ digits, and is optional '(?:^|,)' # match start of string or a comma