Conversation
- Changed Python version requirement from ^3.9 to ^3.10. - Updated ruff dependency to the latest version. - Adjusted target version for ruff from py39 to py310. - Removed unnecessary linting ignores for missing type hints on self and cls. - Cleaned up formatting in the pyproject.toml file.
There was a problem hiding this comment.
Pull request overview
This pull request updates the Confluence CLI project with maintenance improvements and a new HTML/XML formatting feature. The changes remove company-specific references, modernize Python typing syntax, update dependencies, and add functionality to format page content output.
Changes:
- Updates minimum Python version from 3.9 to 3.10 and modernizes type hints to use PEP 604 union syntax (
|instead oftyping.Optional) - Adds
lxmldependency and implements a new--prettyoption for formatting HTML/XML output in theget_bodycommand - Removes company-specific references from documentation and configuration to make the project more generic
Reviewed changes
Copilot reviewed 19 out of 20 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| pyproject.toml | Updates Python version to 3.10, adds lxml dependency, removes ruff version constraint, removes deprecated ruff rules (ANN101/ANN102), and removes poetry-dynamic-versioning format configuration |
| docs/user_guide/getting_started.rst | Updates Python version requirement and removes company-specific Confluence URL reference |
| docs/conf.py | Removes empty footer configuration |
| docs/command_reference/page/get_body.rst | Fixes title, adds documentation for storage format and representation options |
| confluence/page/get_page_body.py | Adds format_html function and --pretty option, modernizes imports, adds BodyRepresentation enum documentation |
| confluence/page/update_page.py | Refactors imports to use relative import for cli module |
| confluence/page/subcommand.py | Modernizes type hints and refactors imports |
| confluence/local/subcommand.py | Removes Optional import and modernizes type hints |
| confluence/content/subcommand.py | Modernizes type hints and refactors imports |
| confluence/content/get_content_by_id.py | Refactors imports to use relative import |
| confluence/common/utils.py | Modernizes type hints |
| confluence/common/cli.py | Modernizes type hints and uses removesuffix() method |
| confluence/common/api.py | Modernizes type hints throughout API class methods |
| confluence/attachment/subcommand.py | Modernizes type hints and refactors imports |
| confluence/attachment/get_attachment.py | Refactors imports to use relative import |
| confluence/attachment/delete_attachment.py | Refactors imports and adds spacing around + operator in f-strings |
| confluence/attachment/create_attachment.py | Refactors imports and adds spacing around + operator in f-strings |
| confluence/main.py | Removes Optional import and modernizes type hints |
| README.md | Removes company-specific description, updates CI badge, and changes example page ID |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| HTMLを整形します。 | ||
|
|
||
| Args: | ||
| content: 整形対象のHTML文字列 | ||
|
|
||
| Returns: | ||
| 整形されたHTML文字列 | ||
| """ | ||
| try: | ||
| # HTMLフラグメントをパースして整形 | ||
| root = html.fromstring(content) | ||
| etree.indent(root, space=" ") | ||
| return html.tostring(root, encoding="unicode") | ||
| except Exception: | ||
| logger.warning("HTML整形に失敗しました。", exc_info=True) |
There was a problem hiding this comment.
The function name format_html and its documentation state it formats HTML, but the help text on line 79 mentions "XMLまたはHTMLを整形して出力します" (formats XML or HTML). However, the implementation uses html.fromstring() which is specifically for parsing HTML, not XML. For the STORAGE representation (which is XML format according to line 23), this function may not properly format the content and could fail or produce incorrect results. Consider either:
- Renaming to clarify it only handles HTML representations, or
- Updating the implementation to detect and handle XML content separately using
etree.fromstring()for XML content
| HTMLを整形します。 | |
| Args: | |
| content: 整形対象のHTML文字列 | |
| Returns: | |
| 整形されたHTML文字列 | |
| """ | |
| try: | |
| # HTMLフラグメントをパースして整形 | |
| root = html.fromstring(content) | |
| etree.indent(root, space=" ") | |
| return html.tostring(root, encoding="unicode") | |
| except Exception: | |
| logger.warning("HTML整形に失敗しました。", exc_info=True) | |
| XMLまたはHTMLを整形します。 | |
| Args: | |
| content: 整形対象のXMLまたはHTML文字列 | |
| Returns: | |
| 整形されたXMLまたはHTML文字列 | |
| """ | |
| try: | |
| # まずXMLとしてパースを試みる（STORAGE表現など） | |
| try: | |
| root = etree.fromstring(content) | |
| etree.indent(root, space=" ") | |
| return etree.tostring(root, encoding="unicode") | |
| except etree.XMLSyntaxError: | |
| # XMLとしてパースできない場合はHTMLとしてパースして整形 | |
| root = html.fromstring(content) | |
| etree.indent(root, space=" ") | |
| return html.tostring(root, encoding="unicode") | |
| except Exception: | |
| logger.warning("XML/HTML整形に失敗しました。", exc_info=True) |
| def format_html(content: str) -> str: | ||
| """ | ||
| HTMLを整形します。 | ||
|
|
||
| Args: | ||
| content: 整形対象のHTML文字列 | ||
|
|
||
| Returns: | ||
| 整形されたHTML文字列 | ||
| """ | ||
| try: | ||
| # HTMLフラグメントをパースして整形 | ||
| root = html.fromstring(content) | ||
| etree.indent(root, space=" ") | ||
| return html.tostring(root, encoding="unicode") | ||
| except Exception: | ||
| logger.warning("HTML整形に失敗しました。", exc_info=True) | ||
| return content |
There was a problem hiding this comment.
The new format_html function and --pretty option lack test coverage. Since the repository has test infrastructure (tests directory with pytest), and there's a test for the local module's convert functionality, new utility functions like format_html should have corresponding tests to verify correct behavior, especially for edge cases and error handling.
| """HTML representation for viewing, but with absolute urls, instead of relative urls in the markup, and macros are rendered as though it is viewed by an anonymous user.""" # noqa: E501 | ||
|
|
||
|
|
||
| def format_html(content: str) -> str: |
There was a problem hiding this comment.
The function is named format_html but the help text on line 79 and the docstring on line 38 claim it handles both XML and HTML. This creates ambiguity about the function's actual purpose. Either rename the function to better reflect what it actually does (e.g., format_content if it should handle both, or keep format_html if it only handles HTML), or update the documentation to accurately describe its capabilities.
| HTMLを整形します。 | ||
|
|
||
| Args: | ||
| content: 整形対象のHTML文字列 | ||
|
|
||
| Returns: | ||
| 整形されたHTML文字列 |
There was a problem hiding this comment.
The documentation incorrectly states the function formats HTML ("HTMLを整形します"), but according to the help text on line 79, it should format both XML and HTML. Update the docstring to accurately reflect that the function handles both XML and HTML content, not just HTML.
| HTMLを整形します。 | |
| Args: | |
| content: 整形対象のHTML文字列 | |
| Returns: | |
| 整形されたHTML文字列 | |
| XMLまたはHTMLを整形します。 | |
| Args: | |
| content: 整形対象のXMLまたはHTML文字列 | |
| Returns: | |
| 整形されたXMLまたはHTML文字列 |
| root = html.fromstring(content) | ||
| etree.indent(root, space=" ") | ||
| return html.tostring(root, encoding="unicode") | ||
| except Exception: |
There was a problem hiding this comment.
The broad exception handler catches all exceptions without specifying which exceptions are expected. This makes it difficult to distinguish between expected parsing errors and unexpected errors (e.g., bugs in the code). Consider catching specific exceptions like lxml.etree.XMLSyntaxError, lxml.etree.ParserError, or ValueError that are expected during HTML parsing, and let other unexpected exceptions propagate.
| except Exception: | |
| except (etree.XMLSyntaxError, etree.ParserError, ValueError): |
No description provided.