Add environment setup script and documentation #1
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
|
⏳ I'm reviewing this pull request for security vulnerabilities and code quality issues. I'll provide an update when I'm done |
Reviewer's GuideThis PR introduces a cross-platform environment setup script, refactors the DOCX template and converter APIs for improved classmethod usage and argument handling, enhances CLI options consistency, updates documentation and CI to integrate the setup, and aligns tests with these API changes. Sequence diagram for CLI option parsing and conversionsequenceDiagram
actor User
participant CLI
participant MarkdownToDocxConverter
User->>CLI: Run markdown2docx CLI with --toc and --toc-depth/--toc_depth
CLI->>CLI: Parse options (toc, toc_depth)
CLI->>MarkdownToDocxConverter: convert(input, output, toc=True, toc_depth=3)
MarkdownToDocxConverter->>MarkdownToDocxConverter: Build Pandoc args
MarkdownToDocxConverter->>MarkdownToDocxConverter: Perform conversion
MarkdownToDocxConverter-->>CLI: Return output path
CLI-->>User: Output DOCX file path
Class diagram for updated DocxTemplateManager and MarkdownToDocxConverterclassDiagram
class DocxTemplateManager {
+heading_font
+code_font
+create_modern_template(output_path, add_sample=False, **kwargs) Path
+_create_modern_template(output_path, add_sample=False) Path
+_configure_core_styles(doc)
+_set_compatibility_mode_xml(doc, mode="16")
+create_default_template(output_path) Path
}
class MarkdownToDocxConverter {
+reference_doc
+min_pandoc
+convert(input_path, output_path=None, **options) Path
+convert_with_template(input_path, template_path, output_path=None, **kwargs) Path
+_get_modern_docx_args() list[str]
+_build_args(toc, toc_depth, extra_args) list[str]
+_validate_docx(path)
}
DocxTemplateManager <.. MarkdownToDocxConverter : uses template
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @cnkang - I've reviewed your changes - here's some feedback:
- The
log.warningcall in converter._build_args isn’t accompanied by alogimport or logger definition—ensure you import/configure your logger to avoid runtime errors. - Renaming the CLI option keys from dashed ("--toc") to underscored ("toc") is a breaking API change; consider aliasing the old keys or clearly bumping the major version.
- The new setup-env.sh script is POSIX‐only and won’t run on Windows—if cross‐platform support is required, consider adding a PowerShell alternative or noting the platform limitation in the docs.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `log.warning` call in converter._build_args isn’t accompanied by a `log` import or logger definition—ensure you import/configure your logger to avoid runtime errors.
- Renaming the CLI option keys from dashed ("--toc") to underscored ("toc") is a breaking API change; consider aliasing the old keys or clearly bumping the major version.
- The new setup-env.sh script is POSIX‐only and won’t run on Windows—if cross‐platform support is required, consider adding a PowerShell alternative or noting the platform limitation in the docs.
## Individual Comments
### Comment 1
<location> `scripts/setup-env.sh:1` </location>
<code_context>
+#!/usr/bin/env bash
+set -e
+
+# Install Python dependencies using uv, allowing additional arguments (e.g., --group dev)
+uv sync "$@"
+
+# Ensure pandoc is installed
+if ! command -v pandoc >/dev/null 2>&1; then
+ echo "Pandoc not found. Attempting to install..."
+ if command -v apt-get >/dev/null 2>&1; then
+ sudo apt-get update && sudo apt-get install -y pandoc
+ elif command -v brew >/dev/null 2>&1; then
+ brew install pandoc
+ elif command -v yum >/dev/null 2>&1; then
+ sudo yum install -y pandoc
+ elif command -v dnf >/dev/null 2>&1; then
+ sudo dnf install -y pandoc
+ elif command -v pacman >/dev/null 2>&1; then
+ sudo pacman -Sy --noconfirm pandoc
+ else
+ echo "Could not determine package manager. Please install pandoc manually."
+ exit 1
+ fi
+else
+ echo "Pandoc already installed."
</code_context>
<issue_to_address>
The script attempts to install Pandoc using various package managers, but may fail on systems without sudo or with non-standard setups.
The script should check for 'sudo' before attempting installation, and offer guidance for manual installation if 'sudo' is unavailable or permissions are limited.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+225
to
+228
| for table in doc.tables: | ||
| for row in table.rows: | ||
| for cell in row.cells: | ||
| full_text += '\n' + cell.text |
There was a problem hiding this comment.
issue (code-quality): Avoid loops in tests. (no-loop-in-tests)
Explanation
Avoid complex code, like loops, in test functions.Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
- loops
- conditionals
Some ways to fix this:
- Use parametrized tests to get rid of the loop.
- Move the complex logic into helpers.
- Move the complex part into pytest fixtures.
Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / Don't Put Logic in Tests
![amazon-q-developer[bot]](https://avatars.githubusercontent.com/in/1220912?s=60&v=4){kind=small}
Contributor
|
✅ I finished the code review, and left comments with the issues I found. I will now generate code fix suggestions. |
Co-authored-by: amazon-q-developer[bot] <208079219+amazon-q-developer[bot]@users.noreply.github.com>
Co-authored-by: amazon-q-developer[bot] <208079219+amazon-q-developer[bot]@users.noreply.github.com>
Add basic logging configuration to ensure logger warnings work properly when reference doc is missing
Add --toc_depth alias for --toc-depth to maintain API compatibility and prevent breaking changes
- Add sudo availability check before package manager operations - Prioritize brew (no sudo required) over other package managers - Provide clear manual installation instructions when sudo unavailable - Include direct link to Pandoc installation guide
Add note that setup script is designed for Unix-like systems and Windows users should install dependencies manually
Owner
Author
|
@sourcery-ai review |
Remove duplicated code and fix unclosed parentheses in run_cli_command function that was causing test collection to fail
There was a problem hiding this comment.
Hey @cnkang - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `scripts/setup-env.sh:5` </location>
<code_context>
+set -e
+
+# Install Python dependencies using uv, allowing additional arguments (e.g., --group dev)
+uv sync "$@"
+
+# Ensure pandoc is installed
</code_context>
<issue_to_address>
The script assumes 'uv' is installed and available in PATH.
Add a check to verify 'uv' is installed, and display a clear error or installation guidance if not found.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
set -e
# Install Python dependencies using uv, allowing additional arguments (e.g., --group dev)
uv sync "$@"
=======
set -e
# Check if 'uv' is installed
if ! command -v uv >/dev/null 2>&1; then
echo "Error: 'uv' is not installed or not found in your PATH."
echo "Please install 'uv' by following instructions at https://github.com/astral-sh/uv or run:"
echo " pip install uv"
exit 1
fi
# Install Python dependencies using uv, allowing additional arguments (e.g., --group dev)
uv sync "$@"
>>>>>>> REPLACE
</suggested_fix>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Verify 'uv' is installed before attempting to use it and provide clear installation guidance if not found
Owner
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey @cnkang - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `tests/test_templates.py` </location>
<code_context>
-def test_template_sample_content():
</code_context>
<issue_to_address>
Consider testing template creation with custom heading styles via kwargs.
Add a test that creates a template using custom heading fonts or sizes through kwargs, and verify these customizations appear in the generated DOCX file.
</issue_to_address>
### Comment 2
<location> `tests/test_integration.py` </location>
<code_context>
-def test_toc_generation(complex_markdown):
</code_context>
<issue_to_address>
Consider adding integration tests for missing or invalid template files.
An integration test should verify that when a non-existent template is used, a warning is logged and conversion completes successfully.
</issue_to_address>
### Comment 3
<location> `tests/test_converter.py:107` </location>
<code_context>
# Conversion with options
output_path = converter.convert(
- "input.md",
+ "input.md",
</code_context>
<issue_to_address>
Consider adding tests for the new convert_with_template method.
Directly testing convert_with_template will help verify its correctness and maintainability.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
…ndling - Add test for template creation with custom heading styles via kwargs - Add integration test for missing template file handling with logging verification - Add direct test for convert_with_template method to ensure correctness
Owner
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey @cnkang - I've reviewed your changes - here's some feedback:
- Move the logging.basicConfig call out of the converter module into the CLI entrypoint (or a dedicated setup function) so that importing the library doesn’t reconfigure the root logger unexpectedly.
- You can simplify the CLI option mapping by relying on argparse’s built-in alias handling for --toc-depth/--toc_depth and drop the getattr(...) fallback logic in main().
- Since setup-env.sh is promoted as cross-platform but only handles Unix-style package managers, consider adding Windows support (e.g. Chocolatey) or clearly documenting that Windows users must install Pandoc manually.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Move the logging.basicConfig call out of the converter module into the CLI entrypoint (or a dedicated setup function) so that importing the library doesn’t reconfigure the root logger unexpectedly.
- You can simplify the CLI option mapping by relying on argparse’s built-in alias handling for --toc-depth/--toc_depth and drop the getattr(...) fallback logic in main().
- Since setup-env.sh is promoted as cross-platform but only handles Unix-style package managers, consider adding Windows support (e.g. Chocolatey) or clearly documenting that Windows users must install Pandoc manually.
## Individual Comments
### Comment 1
<location> `src/markdown2docx/converter.py:18` </location>
<code_context>
Version = None # Converter still works without packaging; only strict compare is skipped
+# Configure logger
+logging.basicConfig(level=logging.INFO)
log = logging.getLogger(__name__)
</code_context>
<issue_to_address>
Setting logging.basicConfig at module level may interfere with application-wide logging configuration.
This can cause conflicts with user-defined logging. It's best to configure logging only in the main entry point or CLI, not in library modules.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Use actual DocxTemplateManager parameters (heading_font, code_font) instead of non-existent heading1_size parameter
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Testing
./scripts/setup-env.sh --group devuv run pytesthttps://chatgpt.com/codex/tasks/task_e_6897d6cb8a60832b81b11a8e82b373d5
Summary by Sourcery
Add environment setup script with documentation, improve template and converter APIs, refine CLI options, and update tests and CI to support new workflow.
New Features:
Enhancements:
CI:
Documentation:
Tests: