From 298caa6a9aca18422f79a7ed5477bc1751564a9b Mon Sep 17 00:00:00 2001 From: James Knight Date: Sat, 2 Mar 2024 14:08:37 -0500 Subject: [PATCH] update exception usage Updating a series of implementation around the handling and raising of exceptions. Specifically, these changes aim to: - Avoid using raw strings to avoid excessive traceback content (EM101). - Use exception chaining when possible. - Remove unneeded parenthesis for no-argument raises. Signed-off-by: James Knight --- sphinxcontrib/confluencebuilder/builder.py | 3 +- .../confluencebuilder/config/__init__.py | 6 +- .../confluencebuilder/config/validation.py | 90 +++++++------- sphinxcontrib/confluencebuilder/directives.py | 37 +++--- sphinxcontrib/confluencebuilder/publisher.py | 114 ++++++++---------- sphinxcontrib/confluencebuilder/rest.py | 30 ++--- sphinxcontrib/confluencebuilder/state.py | 8 +- .../confluencebuilder/storage/translator.py | 5 +- tests/lib/__init__.py | 5 +- 9 files changed, 151 insertions(+), 147 deletions(-) diff --git a/sphinxcontrib/confluencebuilder/builder.py b/sphinxcontrib/confluencebuilder/builder.py index 157782d3..cdaa1654 100644 --- a/sphinxcontrib/confluencebuilder/builder.py +++ b/sphinxcontrib/confluencebuilder/builder.py @@ -1014,7 +1014,8 @@ def _header_footer_init(self, docname, doctree): else: # unsupported source type should not pass here after this # extension's configuration check - raise AssertionError('unsupported source type') + msg = 'unsupported source type' + raise AssertionError(msg) sourcelink['url'] = url_base + url diff --git a/sphinxcontrib/confluencebuilder/config/__init__.py b/sphinxcontrib/confluencebuilder/config/__init__.py index afe8597c..993e4e07 100644 --- a/sphinxcontrib/confluencebuilder/config/__init__.py +++ b/sphinxcontrib/confluencebuilder/config/__init__.py @@ -58,7 +58,8 @@ def process_ask_configs(config): target_user = input(f' User{u_str}: ') or default_user if not target_user: - raise ConfluenceConfigError('no user provided') + msg = 'no user provided' + raise ConfluenceConfigError(msg) config.confluence_server_user = target_user @@ -71,4 +72,5 @@ def process_ask_configs(config): sys.stdout.flush() config.confluence_server_pass = util.getpass2('') if not config.confluence_server_pass: - raise ConfluenceConfigError('no password provided') + msg = 'no password provided' + raise ConfluenceConfigError(msg) diff --git a/sphinxcontrib/confluencebuilder/config/validation.py b/sphinxcontrib/confluencebuilder/config/validation.py index 2cd220b6..d5cbad71 100644 --- a/sphinxcontrib/confluencebuilder/config/validation.py +++ b/sphinxcontrib/confluencebuilder/config/validation.py @@ -56,12 +56,12 @@ def bool(self): if isinstance(value, str) or isinstance(value, int): try: str2bool(value) - except ValueError: - raise ConfluenceConfigError( - '%s is not a boolean string' % self.key) + except ValueError as ex: + msg = f'{self.key} is not a boolean string' + raise ConfluenceConfigError(msg) from ex elif not isinstance(value, bool) and not isinstance(value, int): - raise ConfluenceConfigError( - '%s is not a boolean type' % self.key) + msg = f'{self.key} is not a boolean type' + raise ConfluenceConfigError(msg) return self @@ -83,8 +83,8 @@ def callable_(self): value = self._value() if value is not None and not callable(value): - raise ConfluenceConfigError( - '%s is not a callable' % self.key) + msg = f'{self.key} is not a callable' + raise ConfluenceConfigError(msg) return self @@ -130,8 +130,8 @@ def dict_str_str(self): if value is not None: if not isinstance(value, dict) or not all(isinstance(k, str) and isinstance(v, str) for k, v in value.items()): - raise ConfluenceConfigError( - '%s is not a dictionary of strings' % self.key) + msg = f'{self.key} is not a dictionary of strings' + raise ConfluenceConfigError(msg) return self @@ -155,15 +155,15 @@ def docnames(self): if value is not None: if not (isinstance(value, (list, set, tuple))) or not all( isinstance(label, (str, os.PathLike)) for label in value): - raise ConfluenceConfigError( - '%s is not a collection of filenames' % self.key) + msg = f'{self.key} is not a collection of filenames' + raise ConfluenceConfigError(msg) for docname in value: if not any( Path(self.env.srcdir, docname + suffix).is_file() for suffix in self.config.source_suffix): - raise ConfluenceConfigError( - f'{self.key} is missing document {docname}') + msg = f'{self.key} is missing document {docname}' + raise ConfluenceConfigError(msg) return self @@ -193,8 +193,8 @@ def docnames_from_file(self): if not any( Path(self.env.srcdir, docname + suffix).is_file() for suffix in self.config.source_suffix): - raise ConfluenceConfigError( - f'{self.key} is missing document {docname}') + msg = f'{self.key} is missing document {docname}' + raise ConfluenceConfigError(msg) return self @@ -220,8 +220,8 @@ def enum(self, etype): try: value = etype[value.lower()] except (AttributeError, KeyError): - raise ConfluenceConfigError( - f'{self.key} is not an enumeration ({etype.__name__})') + msg = f'{self.key} is not an enumeration ({etype.__name__})' + raise ConfluenceConfigError(msg) return self @@ -245,8 +245,8 @@ def file(self): if value is not None: if not isinstance(value, (str, os.PathLike)) or \ not Path(self.env.srcdir, value).is_file(): - raise ConfluenceConfigError( - '%s is not a file' % self.key) + msg = f'{self.key} is not a file' + raise ConfluenceConfigError(msg) return self @@ -276,18 +276,18 @@ def float_(self, positive=False): try: value = float(value) except ValueError: - raise ConfluenceConfigError( - '%s is not a float string' % self.key) + msg = f'{self.key} is not a float string' + raise ConfluenceConfigError(msg) elif isinstance(value, int): value = float(value) if positive: if not isinstance(value, float) or value <= 0: - raise ConfluenceConfigError( - '%s is not a positive float' % self.key) + msg = f'{self.key} is not a positive float' + raise ConfluenceConfigError() elif not isinstance(value, float) or value < 0: - raise ConfluenceConfigError( - '%s is not a non-negative float' % self.key) + msg = f'{self.key} is not a non-negative float' + raise ConfluenceConfigError(msg) return self @@ -317,16 +317,16 @@ def int_(self, positive=False): try: value = int(value) except ValueError: - raise ConfluenceConfigError( - '%s is not an integer string' % self.key) + msg = f'{self.key} is not an integer string' + raise ConfluenceConfigError(msg) if positive: if not isinstance(value, int) or value <= 0: - raise ConfluenceConfigError( - '%s is not a positive integer' % self.key) + msg = f'{self.key} is not a positive integer' + raise ConfluenceConfigError(msg) elif not isinstance(value, int) or value < 0: - raise ConfluenceConfigError( - '%s is not a non-negative integer' % self.key) + msg = f'{self.key} is not a non-negative integer' + raise ConfluenceConfigError(msg) return self @@ -351,8 +351,8 @@ def matching(self, *expected): value = self._value() if value is not None and value not in expected: - raise ConfluenceConfigError( - '%s does not match expected values' % self.key) + msg = f'{self.key} does not match expected values' + raise ConfluenceConfigError(msg) return self @@ -376,8 +376,8 @@ def path(self): if value is not None: if not isinstance(value, (str, os.PathLike)) or \ not Path(self.env.srcdir, value).exists(): - raise ConfluenceConfigError( - '%s is not a path' % self.key) + msg = f'{self.key} is not a path' + raise ConfluenceConfigError(msg) return self @@ -400,8 +400,8 @@ def string(self): value = self._value() if value is not None and not isinstance(value, str): - raise ConfluenceConfigError( - '%s is not a string' % self.key) + msg = f'{self.key} is not a string' + raise ConfluenceConfigError(msg) return self @@ -425,11 +425,11 @@ def string_or_strings(self): if value is not None: if isinstance(value, (list, set, tuple)): if not all(isinstance(entry, str) for entry in value): - raise ConfluenceConfigError( - '%s is not a collection of strings' % self.key) + msg = f'{self.key} is not a collection of strings' + raise ConfluenceConfigError(msg) elif not isinstance(value, str): - raise ConfluenceConfigError( - '%s is not a string or collection of strings' % self.key) + msg = f'{self.key} is not a string or collection of strings' + raise ConfluenceConfigError(msg) return self @@ -457,14 +457,14 @@ def strings(self, no_space=False): if value is not None: if not (isinstance(value, (list, set, tuple)) and all( isinstance(entry, str) for entry in value)): - raise ConfluenceConfigError( - '%s is not a collection of strings' % self.key) + msg = f'{self.key} is not a collection of strings' + raise ConfluenceConfigError(msg) if no_space: for entry in value: if ' ' in entry: - raise ConfluenceConfigError( - '%s has an entry containing a space' % self.key) + msg = f'{self.key} has an entry containing a space' + raise ConfluenceConfigError(msg) return self diff --git a/sphinxcontrib/confluencebuilder/directives.py b/sphinxcontrib/confluencebuilder/directives.py index 0ddeaea5..f6385759 100644 --- a/sphinxcontrib/confluencebuilder/directives.py +++ b/sphinxcontrib/confluencebuilder/directives.py @@ -84,7 +84,7 @@ def run(self): return [node] def _build_card_node(self): - raise NotImplementedError() + raise NotImplementedError class ConfluenceDocDirective(ConfluenceCardDirective): @@ -260,41 +260,48 @@ def run(self): # if explicit server is provided, ensure both options are set if 'server' in params or 'serverId' in params: if 'server' not in params: - raise self.error(':server-name: required when server-id is ' - 'set; but none supplied') + msg = ':server-name: required when server-id is ' \ + 'set; but none supplied' + raise self.error(msg) if 'serverId' not in params: - raise self.error(':server-id: required when server-name is ' - 'set; but none supplied') + msg = ':server-id: required when server-name is ' \ + 'set; but none supplied' + raise self.error(msg) # if a server key is provided, fetch values from configuration elif target_server: config = self.state.document.settings.env.config if not config.confluence_jira_servers: - raise self.error(':server: is set but no ' - 'confluence_jira_servers defined in config') + msg = ':server: is set but no ' \ + 'confluence_jira_servers defined in config' + raise self.error(msg) jira_servers = config['confluence_jira_servers'] if target_server not in jira_servers: - raise self.error(':server: is set but does not exist in ' - 'confluence_jira_servers config') + msg = ':server: is set but does not exist in ' \ + 'confluence_jira_servers config' + raise self.error(msg) jira_server_config = jira_servers[target_server] if 'name' not in jira_server_config: - raise self.error(':server: is set but missing name entry in ' - 'confluence_jira_servers config') + msg = ':server: is set but missing name entry in ' \ + 'confluence_jira_servers config' + raise self.error(msg) params['server'] = jira_server_config['name'] if 'id' not in jira_server_config: - raise self.error(':server: is set but missing id entry in ' - 'confluence_jira_servers config') + msg = ':server: is set but missing id entry in ' \ + 'confluence_jira_servers config' + raise self.error(msg) params['serverId'] = jira_server_config['id'] if 'serverId' in params: try: UUID(params['serverId'], version=4) except ValueError: - raise self.error('server-id is not a valid uuid') + msg = 'server-id is not a valid uuid' + raise self.error(msg) return [node] def _build_jira_node(self): - raise NotImplementedError() + raise NotImplementedError class JiraDirective(JiraBaseDirective): diff --git a/sphinxcontrib/confluencebuilder/publisher.py b/sphinxcontrib/confluencebuilder/publisher.py index 6bc67fa1..1372678e 100644 --- a/sphinxcontrib/confluencebuilder/publisher.py +++ b/sphinxcontrib/confluencebuilder/publisher.py @@ -68,13 +68,13 @@ def connect(self): try: rsp = self.rest_client.get(f'space/{self.space_key}') - except ConfluenceBadApiError as e: - raise ConfluenceBadServerUrlError(server_url, e) + except ConfluenceBadApiError as ex: + raise ConfluenceBadServerUrlError(server_url, ex) from ex # sanity check that we have a sane response if not isinstance(rsp, dict): - raise ConfluenceBadServerUrlError(server_url, - 'server did not provide an expected response; no dictionary') + msg = 'server did not provide an expected response; no dictionary' + raise ConfluenceBadServerUrlError(server_url, msg) expected_entries = [ 'id', @@ -84,14 +84,14 @@ def connect(self): ] if not all(entry in rsp for entry in expected_entries): - raise ConfluenceBadServerUrlError(server_url, - 'server did not provide an expected response; missing entries') + msg = 'server did not provide an expected response; missing entries' + raise ConfluenceBadServerUrlError(server_url, msg) detected_key = rsp['key'] if detected_key != self.space_key: - raise ConfluenceBadServerUrlError(server_url, - 'server did not provide an expected response; bad key match; ' - f'{detected_key} != {self.space_key}') + msg = 'server did not provide an expected response; ' \ + f'bad key match; {detected_key} != {self.space_key}' + raise ConfluenceBadServerUrlError(server_url, msg) # track required space information self.space_display_name = rsp['name'] @@ -128,13 +128,12 @@ def archive_page(self, page_id): attempt += 1 if attempt > MAX_WAIT_FOR_PAGE_ARCHIVE: - raise ConfluenceBadApiError( - -1, 'timeout waiting for archive completion') - except ConfluencePermissionError: - raise ConfluencePermissionError( - """Publish user does not have permission to archive """ - """from the configured space.""" - ) + msg = 'timeout waiting for archive completion' + raise ConfluenceBadApiError(-1, msg) + except ConfluencePermissionError as ex: + msg = 'Publish user does not have permission to archive ' \ + 'from the configured space.' + raise ConfluencePermissionError(msg) from ex def archive_pages(self, page_ids): if self.dryrun: @@ -157,11 +156,10 @@ def archive_pages(self, page_ids): # Cannot use bulk archive feature for non premium edition self.rest_client.post('content/archive', data) - except ConfluencePermissionError: - raise ConfluencePermissionError( - """Publish user does not have permission to archive """ - """from the configured space.""" - ) + except ConfluencePermissionError as ex: + msg = 'Publish user does not have permission to archive ' \ + 'from the configured space.' + raise ConfluencePermissionError(msg) from ex def get_ancestors(self, page_id): """ @@ -198,8 +196,8 @@ def get_base_page_id(self): base_page_id, page = self.get_page_by_id(self.parent_ref) if not page: - raise ConfluenceConfigError( - '''Configured parent page identifier does not exist.''') + msg = 'Configured parent page identifier does not exist.' + raise ConfluenceConfigError(msg) return base_page_id @@ -210,18 +208,18 @@ def get_base_page_id(self): 'status': 'current', }) if rsp['size'] == 0: - raise ConfluenceConfigError( - '''Configured parent page name does not exist.''') + msg = 'Configured parent page name does not exist.' + raise ConfluenceConfigError(msg) page = rsp['results'][0] if self.parent_id and page['id'] != str(self.parent_id): - raise ConfluenceConfigError("""Configured parent """ - """page ID and name do not match.""") + msg = 'Configured parent page ID and name do not match.' + raise ConfluenceConfigError(msg) base_page_id = page['id'] self._name_cache[base_page_id] = self.parent_ref if not base_page_id and self.parent_id: - raise ConfluenceConfigError("""Unable to find the """ - """parent page matching the ID or name provided.""") + msg = 'Unable to find parent page matching the ID/name provided.' + raise ConfluenceConfigError(msg) return base_page_id @@ -694,11 +692,10 @@ def store_attachment(self, page_id, name, data, mimetype, hash_, force=False): if not self.watch: self.rest_client.delete('user/watch/content', uploaded_attachment_id) - except ConfluencePermissionError: - raise ConfluencePermissionError( - """Publish user does not have permission to add an """ - """attachment to the configured space.""" - ) + except ConfluencePermissionError as ex: + msg = 'Publish user does not have permission to add an ' \ + 'attachment to the configured space.' + raise ConfluencePermissionError(msg) from ex return uploaded_attachment_id @@ -910,11 +907,10 @@ def store_page(self, page_name, data, parent_id=None): props['value']['hash'] = new_page_hash self.store_page_properties(uploaded_page_id, props) - except ConfluencePermissionError: - raise ConfluencePermissionError( - """Publish user does not have permission to add page """ - """content to the configured space.""" - ) + except ConfluencePermissionError as ex: + msg = 'Publish user does not have permission to add page ' \ + 'content to the configured space.' + raise ConfluencePermissionError(msg) from ex if not self.watch: self.rest_client.delete('user/watch/content', uploaded_page_id) @@ -958,15 +954,14 @@ def store_page_by_id(self, page_name, page_id, data): except ConfluenceBadApiError as ex: if str(ex).find('No content found with id') == -1: raise - raise ConfluenceMissingPageIdError(self.space_key, page_id) + raise ConfluenceMissingPageIdError(self.space_key, page_id) from ex try: self._update_page(page, page_name, data) - except ConfluencePermissionError: - raise ConfluencePermissionError( - """Publish user does not have permission to add page """ - """content to the configured space.""" - ) + except ConfluencePermissionError as ex: + msg = 'Publish user does not have permission to add page ' \ + 'content to the configured space.' + raise ConfluencePermissionError(msg) from ex if not self.watch: self.rest_client.delete('user/watch/content', page_id) @@ -1007,11 +1002,10 @@ def remove_attachment(self, id_): try: self.rest_client.delete('content', id_) - except ConfluencePermissionError: - raise ConfluencePermissionError( - """Publish user does not have permission to delete """ - """from the configured space.""" - ) + except ConfluencePermissionError as ex: + msg = 'Publish user does not have permission to delete ' \ + 'from the configured space.' + raise ConfluencePermissionError(msg) from ex def remove_page(self, page_id): if self.dryrun: @@ -1044,11 +1038,10 @@ def remove_page(self, page_id): logger.verbose('ignore missing delete for page ' f'identifier: {page_id}') - except ConfluencePermissionError: - raise ConfluencePermissionError( - """Publish user does not have permission to delete """ - """from the configured space.""" - ) + except ConfluencePermissionError as ex: + msg = 'Publish user does not have permission to delete ' \ + 'from the configured space.' + raise ConfluencePermissionError(msg) from ex def restrict_ancestors(self, ancestors): """ @@ -1084,11 +1077,10 @@ def update_space_home(self, page_id): 'homepage': page, 'type': self.space_type, }) - except ConfluencePermissionError: - raise ConfluencePermissionError( - """Publish user does not have permission to update """ - """space's homepage.""" - ) + except ConfluencePermissionError as ex: + msg = 'Publish user does not have permission to update ' \ + "space's homepage." + raise ConfluencePermissionError(msg) from ex def _build_page(self, page_name, data): """ @@ -1221,7 +1213,7 @@ def _update_page(self, page, page_name, data, parent_id=None): except ConfluenceBadApiError as ex: if 'unreconciled' in str(ex): raise ConfluenceUnreconciledPageError( - page_name, page['id'], self.server_url, ex) + page_name, page['id'], self.server_url, ex) from ex raise diff --git a/sphinxcontrib/confluencebuilder/rest.py b/sphinxcontrib/confluencebuilder/rest.py index 9751c454..adcad05c 100644 --- a/sphinxcontrib/confluencebuilder/rest.py +++ b/sphinxcontrib/confluencebuilder/rest.py @@ -107,10 +107,10 @@ def _wrapper(self, *args, **kwargs): while True: try: return func(self, *args, **kwargs) - except ConfluenceRateLimitedError as e: + except ConfluenceRateLimitedError: # noqa: PERF203 # if max attempts have been reached, stop any more attempts if attempt > RATE_LIMITED_MAX_RETRIES: - raise e + raise # determine the amount of delay to wait again -- either from the # provided delay (if any) or exponential backoff @@ -149,12 +149,12 @@ def _decorator(func): def _wrapper(self, *args, **kwargs): try: return func(self, *args, **kwargs) - except requests.exceptions.Timeout: - raise ConfluenceTimeoutError(self.url) + except requests.exceptions.Timeout as ex: + raise ConfluenceTimeoutError(self.url) from ex except requests.exceptions.SSLError as ex: - raise ConfluenceSslError(self.url, ex) + raise ConfluenceSslError(self.url, ex) from ex except requests.exceptions.ConnectionError as ex: - raise ConfluenceBadServerUrlError(self.url, ex) + raise ConfluenceBadServerUrlError(self.url, ex) from ex return _wrapper return _decorator @@ -248,9 +248,9 @@ def get(self, path, params=None): try: rsp.encoding = self.CONFLUENCE_DEFAULT_ENCODING json_data = json.loads(rsp.text) - except ValueError: - raise ConfluenceBadServerUrlError(self.url, - "REST reply did not provide valid JSON data.") + except ValueError as ex: + msg = 'REST reply did not provide valid JSON data.' + raise ConfluenceBadServerUrlError(self.url, msg) from ex return json_data @@ -271,9 +271,9 @@ def post(self, path, data, files=None): try: rsp.encoding = self.CONFLUENCE_DEFAULT_ENCODING json_data = json.loads(rsp.text) - except ValueError: - raise ConfluenceBadServerUrlError(self.url, - "REST reply did not provide valid JSON data.") + except ValueError as ex: + msg = 'REST reply did not provide valid JSON data.' + raise ConfluenceBadServerUrlError(self.url, msg) from ex return json_data @@ -294,9 +294,9 @@ def put(self, path, value, data): try: rsp.encoding = self.CONFLUENCE_DEFAULT_ENCODING json_data = json.loads(rsp.text) - except ValueError: - raise ConfluenceBadServerUrlError(self.url, - "REST reply did not provide valid JSON data.") + except ValueError as ex: + msg = 'REST reply did not provide valid JSON data.' + raise ConfluenceBadServerUrlError(self.url, msg) from ex return json_data diff --git a/sphinxcontrib/confluencebuilder/state.py b/sphinxcontrib/confluencebuilder/state.py index 979180bb..ae67d327 100644 --- a/sphinxcontrib/confluencebuilder/state.py +++ b/sphinxcontrib/confluencebuilder/state.py @@ -225,10 +225,10 @@ def _format_postfix(postfix, docname, config): return postfix.format( hash=ConfluenceState._create_docname_unique_hash(docname, config), ) - except KeyError: - raise ConfluenceConfigError( - f"Configured confluence_publish_prefix '{postfix}' has an " - "unknown template replacement.") + except KeyError as ex: + msg = f"Configured confluence_publish_prefix '{postfix}' has " \ + "an unknown template replacement." + raise ConfluenceConfigError(msg) from ex return postfix @staticmethod diff --git a/sphinxcontrib/confluencebuilder/storage/translator.py b/sphinxcontrib/confluencebuilder/storage/translator.py index 50f2ddf0..b281c5f4 100644 --- a/sphinxcontrib/confluencebuilder/storage/translator.py +++ b/sphinxcontrib/confluencebuilder/storage/translator.py @@ -3184,8 +3184,9 @@ def _end_tag(self, node, suffix=None): """ try: tag = node.__confluence_tag.pop() - except IndexError: - raise ConfluenceError('end tag invoke without matching start tag') + except IndexError as ex: + msg = 'end tag invoke without matching start tag' + raise ConfluenceError(msg) from ex if suffix is None: suffix = self.nl diff --git a/tests/lib/__init__.py b/tests/lib/__init__.py index 0d064e89..788b737c 100644 --- a/tests/lib/__init__.py +++ b/tests/lib/__init__.py @@ -74,7 +74,7 @@ def check_unhandled_requests(self): with self.mtx: if self.del_req or self.get_req or self.put_req: - raise Exception(f'''unhandled requests detected + msg = f'''unhandled requests detected (get requests) {self.get_req} @@ -83,7 +83,8 @@ def check_unhandled_requests(self): {self.put_req} (del requests) -{self.del_req}''') +{self.del_req}''' + raise Exception(msg) def pop_delete_request(self): """