Docs API: Use stdout by default. Add YAML output format.#400
Conversation
WalkthroughThis pull request refines the settings extraction functionality in the CrateDB Toolkit. It enhances the CLI to support YAML output in addition to JSON, Markdown, and SQL. The extraction logic has been transformed from a procedural style to an object-oriented approach via the new Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant CLI as CLI Command
participant SE as SettingsExtractor
U->>CLI: Run command (e.g., ctk docs settings --format=yaml)
CLI->>SE: Instantiate SettingsExtractor
SE->>SE: acquire() - Gather settings data
SE->>SE: render(format) - Format settings (JSON, YAML, Markdown, SQL)
SE->>SE: write(output) - Write the formatted output
SE-->>CLI: Return output confirmation
CLI-->>U: Display output message
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🪛 GitHub Check: codecov/patchcratedb_toolkit/docs/settings.py[warning] 15-16: cratedb_toolkit/docs/settings.py#L15-L16 [warning] 21-23: cratedb_toolkit/docs/settings.py#L21-L23 [warning] 26-26: cratedb_toolkit/docs/settings.py#L26 [warning] 32-33: cratedb_toolkit/docs/settings.py#L32-L33 [warning] 49-49: cratedb_toolkit/docs/settings.py#L49 [warning] 54-59: cratedb_toolkit/docs/settings.py#L54-L59 [warning] 62-62: cratedb_toolkit/docs/settings.py#L62 [warning] 428-428: cratedb_toolkit/docs/settings.py#L428 [warning] 435-436: cratedb_toolkit/docs/settings.py#L435-L436 [warning] 438-439: cratedb_toolkit/docs/settings.py#L438-L439 [warning] 442-446: cratedb_toolkit/docs/settings.py#L442-L446 [warning] 449-449: cratedb_toolkit/docs/settings.py#L449 [warning] 451-454: cratedb_toolkit/docs/settings.py#L451-L454 [warning] 456-456: cratedb_toolkit/docs/settings.py#L456 [warning] 459-463: cratedb_toolkit/docs/settings.py#L459-L463 [warning] 465-465: cratedb_toolkit/docs/settings.py#L465 [warning] 467-469: cratedb_toolkit/docs/settings.py#L467-L469 [warning] 471-471: cratedb_toolkit/docs/settings.py#L471 [warning] 473-473: cratedb_toolkit/docs/settings.py#L473 [warning] 592-595: cratedb_toolkit/docs/settings.py#L592-L595 [warning] 597-600: cratedb_toolkit/docs/settings.py#L597-L600 [warning] 603-604: cratedb_toolkit/docs/settings.py#L603-L604 [warning] 606-607: cratedb_toolkit/docs/settings.py#L606-L607 [warning] 609-610: cratedb_toolkit/docs/settings.py#L609-L610 [warning] 613-613: cratedb_toolkit/docs/settings.py#L613 [warning] 616-619: cratedb_toolkit/docs/settings.py#L616-L619 [warning] 622-623: cratedb_toolkit/docs/settings.py#L622-L623 [warning] 629-630: cratedb_toolkit/docs/settings.py#L629-L630 cratedb_toolkit/docs/cli.py[warning] 60-60: cratedb_toolkit/docs/cli.py#L60 [warning] 62-63: cratedb_toolkit/docs/cli.py#L62-L63 ⏰ Context from checks skipped due to timeout of 90000ms (6)
🔇 Additional comments (14)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
doc/docs-api.md (2)
5-8: Add official link or reference to installation docs
While these installation instructions look correct, consider linking to your project's official documentation or referencing a more detailed guide about installing the Docs API, such as in your README or a dedicated installation/integration section.
10-19: Include YAML usage example
You’ve provided examples for JSON, Markdown, and SQL formats. To fully showcase the new YAML format, consider adding a short example demonstrating--format=yaml.cratedb_toolkit/docs/cli.py (1)
60-63: Ensure all new extractor logic is covered by tests
You might be missing test coverage for scenarios invokingSettingsExtractorfrom the CLI (e.g., empty or invalid documentation scenarios). Consider adding negative or edge-case tests to further improve coverage.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 60-60: cratedb_toolkit/docs/cli.py#L60
Added line #L60 was not covered by tests
[warning] 62-63: cratedb_toolkit/docs/cli.py#L62-L63
Added lines #L62 - L63 were not covered by teststests/docs/test_cli.py (1)
31-51: Align file extension with output format
These tests generate Markdown or SQL output yet save to a file namedcratedb-settings.yaml. Renaming the output file to match the actual format (e.g.,.mdfor Markdown,.sqlfor SQL) improves clarity.Apply the following diffs for clarity:
--- a/tests/docs/test_cli.py +++ b/tests/docs/test_cli.py @@ -36,7 +36,7 @@ def test_settings_markdown(tmp_path: Path): - output_path = tmp_path / "cratedb-settings.yaml" + output_path = tmp_path / "cratedb-settings.md" @@ -58,7 +58,7 @@ def test_settings_sql(tmp_path: Path): - output_path = tmp_path / "cratedb-settings.yaml" + output_path = tmp_path / "cratedb-settings.sql"Also applies to: 53-73
cratedb_toolkit/docs/settings.py (2)
614-617: Update docstring to mention YAML output
The docstring says “Output in JSON, Markdown, or SQL format” but YAML is also supported.Use this small fix:
- Output in JSON, Markdown, or SQL format. + Output in JSON, YAML, Markdown, or SQL format.
53-58: Add negative or timeout test coverage
The code block fetching documentation is not covered by tests if the fetch fails or if the returned content is empty. Consider adding a dedicated scenario forcing a non-200 HTTP status or empty response to confirm error handling and logging.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 53-58: cratedb_toolkit/docs/settings.py#L53-L58
Added lines #L53 - L58 were not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cratedb_toolkit/docs/cli.py(2 hunks)cratedb_toolkit/docs/settings.py(4 hunks)doc/docs-api.md(1 hunks)tests/docs/test_cli.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
cratedb_toolkit/docs/cli.py (1)
cratedb_toolkit/docs/settings.py (4)
SettingsExtractor(613-658)acquire(622-632)render(634-648)write(650-658)
🪛 GitHub Check: codecov/patch
cratedb_toolkit/docs/cli.py
[warning] 60-60: cratedb_toolkit/docs/cli.py#L60
Added line #L60 was not covered by tests
[warning] 62-63: cratedb_toolkit/docs/cli.py#L62-L63
Added lines #L62 - L63 were not covered by tests
cratedb_toolkit/docs/settings.py
[warning] 15-16: cratedb_toolkit/docs/settings.py#L15-L16
Added lines #L15 - L16 were not covered by tests
[warning] 21-21: cratedb_toolkit/docs/settings.py#L21
Added line #L21 was not covered by tests
[warning] 25-25: cratedb_toolkit/docs/settings.py#L25
Added line #L25 was not covered by tests
[warning] 31-32: cratedb_toolkit/docs/settings.py#L31-L32
Added lines #L31 - L32 were not covered by tests
[warning] 48-48: cratedb_toolkit/docs/settings.py#L48
Added line #L48 was not covered by tests
[warning] 53-58: cratedb_toolkit/docs/settings.py#L53-L58
Added lines #L53 - L58 were not covered by tests
[warning] 61-61: cratedb_toolkit/docs/settings.py#L61
Added line #L61 was not covered by tests
[warning] 427-427: cratedb_toolkit/docs/settings.py#L427
Added line #L427 was not covered by tests
[warning] 434-435: cratedb_toolkit/docs/settings.py#L434-L435
Added lines #L434 - L435 were not covered by tests
[warning] 437-438: cratedb_toolkit/docs/settings.py#L437-L438
Added lines #L437 - L438 were not covered by tests
[warning] 441-445: cratedb_toolkit/docs/settings.py#L441-L445
Added lines #L441 - L445 were not covered by tests
[warning] 448-448: cratedb_toolkit/docs/settings.py#L448
Added line #L448 was not covered by tests
[warning] 450-453: cratedb_toolkit/docs/settings.py#L450-L453
Added lines #L450 - L453 were not covered by tests
[warning] 455-455: cratedb_toolkit/docs/settings.py#L455
Added line #L455 was not covered by tests
[warning] 458-462: cratedb_toolkit/docs/settings.py#L458-L462
Added lines #L458 - L462 were not covered by tests
[warning] 464-464: cratedb_toolkit/docs/settings.py#L464
Added line #L464 was not covered by tests
[warning] 466-468: cratedb_toolkit/docs/settings.py#L466-L468
Added lines #L466 - L468 were not covered by tests
[warning] 470-470: cratedb_toolkit/docs/settings.py#L470
Added line #L470 was not covered by tests
[warning] 472-472: cratedb_toolkit/docs/settings.py#L472
Added line #L472 was not covered by tests
[warning] 591-594: cratedb_toolkit/docs/settings.py#L591-L594
Added lines #L591 - L594 were not covered by tests
[warning] 596-599: cratedb_toolkit/docs/settings.py#L596-L599
Added lines #L596 - L599 were not covered by tests
[warning] 602-603: cratedb_toolkit/docs/settings.py#L602-L603
Added lines #L602 - L603 were not covered by tests
[warning] 605-606: cratedb_toolkit/docs/settings.py#L605-L606
Added lines #L605 - L606 were not covered by tests
[warning] 608-609: cratedb_toolkit/docs/settings.py#L608-L609
Added lines #L608 - L609 were not covered by tests
[warning] 612-613: cratedb_toolkit/docs/settings.py#L612-L613
Added lines #L612 - L613 were not covered by tests
[warning] 619-620: cratedb_toolkit/docs/settings.py#L619-L620
Added lines #L619 - L620 were not covered by tests
[warning] 622-622: cratedb_toolkit/docs/settings.py#L622
Added line #L622 was not covered by tests
[warning] 624-624: cratedb_toolkit/docs/settings.py#L624
Added line #L624 was not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Generic: Python 3.12 on OS ubuntu-latest
- GitHub Check: Kinesis: Python 3.12 on OS ubuntu-latest
- GitHub Check: Generic: Python 3.9 on OS ubuntu-latest
- GitHub Check: CFR for OS windows-latest
- GitHub Check: Kinesis: Python 3.9 on OS ubuntu-latest
- GitHub Check: CFR for OS ubuntu-latest
- GitHub Check: build-and-test
- GitHub Check: Generic: Python 3.8 on OS ubuntu-latest
- GitHub Check: CFR for OS macos-latest
- GitHub Check: CFR for OS macos-13
🔇 Additional comments (3)
cratedb_toolkit/docs/cli.py (1)
49-51: No concerns – YAML format choice is consistent
The updated format choices and help text properly reflect the addition of YAML.tests/docs/test_cli.py (2)
10-28: JSON test logic looks good
This test accurately validates the JSON output and checks for specific text in the result.
75-94: YAML test appears comprehensive
This test covers the new YAML format thoroughly by asserting the presence of expected text in the parsed YAML. Good job.
amotl
force-pushed
the
docs-settings-stdout
branch
from
b56efa7 to
79a90aa
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
cratedb_toolkit/docs/settings.py (3)
650-658: Consider improving error handling in write methodWhile the error handling for a None payload is good, consider making the method more user-friendly by automatically calling render() with a default format if payload is None instead of raising an error.
def write(self, path: Optional[Path] = None): if self.payload is None: - raise ValueError("No content to write. Please `render()` first.") + logger.info("No content to render, using default JSON format") + self.render("json") if path is None: print(self.payload) # noqa: T201 else: with open(path, "w", encoding="utf-8") as f: f.write(self.payload) return self
634-644: Consider using enum for format typesUsing string literals for format types could lead to errors. Consider using an Enum to represent the supported formats for better type safety and IDE support.
+from enum import Enum, auto + + +class OutputFormat(str, Enum): + """Output formats supported by the SettingsExtractor.""" + + JSON = "json" + YAML = "yaml" + MARKDOWN = "markdown" + SQL = "sql" + @dataclasses.dataclass class SettingsExtractor: ... - def render(self, format_: str): + def render(self, format_: Union[str, OutputFormat]): + # Convert string format to enum if needed + if isinstance(format_, str): + try: + format_ = OutputFormat(format_.lower()) + except ValueError: + raise ValueError(f"Unsupported format: {format_}. Choose from: {', '.join(f.value for f in OutputFormat)}") + # Save to the file in selected format. - if format_ == "json": + if format_ == OutputFormat.JSON: self.payload = json.dumps(self.settings, indent=2, ensure_ascii=False) - elif format_ == "yaml": + elif format_ == OutputFormat.YAML: self.payload = yaml.dump(self.settings) - elif format_ == "markdown": + elif format_ == OutputFormat.MARKDOWN: self.payload = write_markdown_table(self.settings) - elif format_ == "sql": + elif format_ == OutputFormat.SQL: self.payload = write_sql_statements(self.settings)
619-620: Add type annotations to SettingsExtractor class fieldsConsider adding more specific type annotations to improve code clarity.
@dataclasses.dataclass class SettingsExtractor: """ Extract CrateDB settings from documentation. Output in JSON, YAML, Markdown, or SQL format. """ - settings: Dict[str, Dict[str, Any]] = dataclasses.field(default_factory=dict) - payload: Optional[str] = None + settings: Dict[str, Dict[str, Any]] = dataclasses.field(default_factory=dict) + payload: Optional[str] = None🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 619-620: cratedb_toolkit/docs/settings.py#L619-L620
Added lines #L619 - L620 were not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cratedb_toolkit/docs/settings.py(4 hunks)doc/docs-api.md(1 hunks)tests/docs/test_cli.py(1 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
cratedb_toolkit/docs/settings.py
[warning] 15-16: cratedb_toolkit/docs/settings.py#L15-L16
Added lines #L15 - L16 were not covered by tests
[warning] 21-21: cratedb_toolkit/docs/settings.py#L21
Added line #L21 was not covered by tests
[warning] 25-25: cratedb_toolkit/docs/settings.py#L25
Added line #L25 was not covered by tests
[warning] 31-32: cratedb_toolkit/docs/settings.py#L31-L32
Added lines #L31 - L32 were not covered by tests
[warning] 48-48: cratedb_toolkit/docs/settings.py#L48
Added line #L48 was not covered by tests
[warning] 53-58: cratedb_toolkit/docs/settings.py#L53-L58
Added lines #L53 - L58 were not covered by tests
[warning] 61-61: cratedb_toolkit/docs/settings.py#L61
Added line #L61 was not covered by tests
[warning] 427-427: cratedb_toolkit/docs/settings.py#L427
Added line #L427 was not covered by tests
[warning] 434-435: cratedb_toolkit/docs/settings.py#L434-L435
Added lines #L434 - L435 were not covered by tests
[warning] 437-438: cratedb_toolkit/docs/settings.py#L437-L438
Added lines #L437 - L438 were not covered by tests
[warning] 441-445: cratedb_toolkit/docs/settings.py#L441-L445
Added lines #L441 - L445 were not covered by tests
[warning] 448-448: cratedb_toolkit/docs/settings.py#L448
Added line #L448 was not covered by tests
[warning] 450-453: cratedb_toolkit/docs/settings.py#L450-L453
Added lines #L450 - L453 were not covered by tests
[warning] 455-455: cratedb_toolkit/docs/settings.py#L455
Added line #L455 was not covered by tests
[warning] 458-462: cratedb_toolkit/docs/settings.py#L458-L462
Added lines #L458 - L462 were not covered by tests
[warning] 464-464: cratedb_toolkit/docs/settings.py#L464
Added line #L464 was not covered by tests
[warning] 466-468: cratedb_toolkit/docs/settings.py#L466-L468
Added lines #L466 - L468 were not covered by tests
[warning] 470-470: cratedb_toolkit/docs/settings.py#L470
Added line #L470 was not covered by tests
[warning] 472-472: cratedb_toolkit/docs/settings.py#L472
Added line #L472 was not covered by tests
[warning] 591-594: cratedb_toolkit/docs/settings.py#L591-L594
Added lines #L591 - L594 were not covered by tests
[warning] 596-599: cratedb_toolkit/docs/settings.py#L596-L599
Added lines #L596 - L599 were not covered by tests
[warning] 602-603: cratedb_toolkit/docs/settings.py#L602-L603
Added lines #L602 - L603 were not covered by tests
[warning] 605-606: cratedb_toolkit/docs/settings.py#L605-L606
Added lines #L605 - L606 were not covered by tests
[warning] 608-609: cratedb_toolkit/docs/settings.py#L608-L609
Added lines #L608 - L609 were not covered by tests
[warning] 612-613: cratedb_toolkit/docs/settings.py#L612-L613
Added lines #L612 - L613 were not covered by tests
[warning] 619-620: cratedb_toolkit/docs/settings.py#L619-L620
Added lines #L619 - L620 were not covered by tests
[warning] 622-622: cratedb_toolkit/docs/settings.py#L622
Added line #L622 was not covered by tests
[warning] 624-624: cratedb_toolkit/docs/settings.py#L624
Added line #L624 was not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Kinesis: Python 3.12 on OS ubuntu-latest
- GitHub Check: Generic: Python 3.12 on OS ubuntu-latest
- GitHub Check: Kinesis: Python 3.9 on OS ubuntu-latest
- GitHub Check: Generic: Python 3.9 on OS ubuntu-latest
- GitHub Check: CFR for OS windows-latest
- GitHub Check: CFR for OS ubuntu-latest
- GitHub Check: Generic: Python 3.8 on OS ubuntu-latest
- GitHub Check: CFR for OS macos-latest
- GitHub Check: build-and-test
- GitHub Check: CFR for OS macos-13
🔇 Additional comments (15)
doc/docs-api.md (3)
5-8: Great addition of installation instructions!Adding the installation section with
uv pip installcommand makes it easier for users to get started with the Docs API.
10-11: Good structural improvement with the Usage sectionAdding this section improves the document organization, making it more intuitive for readers.
14-15: Nice enhancement to document YAML format supportThe documentation now correctly mentions the addition of YAML as an output format option, aligning with the implementation changes.
tests/docs/test_cli.py (5)
4-4: Good addition of yaml import for the new format supportThe yaml import is correctly added to support the new YAML testing functionality.
10-28: Well-structured JSON format testThe test has been properly updated to explicitly test the JSON output format with appropriate assertions.
31-51: Good test coverage for Markdown formatThis new test properly verifies the Markdown output format by checking for expected content patterns in the generated file.
53-73: Comprehensive SQL format testingThe SQL output format test is thorough and checks for both statement content and summary information.
75-94: Excellent YAML format test implementationThe test appropriately validates the new YAML output format by parsing the output with yaml.safe_load and checking for expected content.
cratedb_toolkit/docs/settings.py (7)
6-6: Updated documentation to include YAML formatDocumentation string correctly updated to include YAML as a supported output format.
15-17: Good imports organization for the new functionalityThe imports have been properly organized to include necessary modules for the expanded functionality, such as io, pathlib, and yaml.
Also applies to: 21-21, 25-25
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 15-16: cratedb_toolkit/docs/settings.py#L15-L16
Added lines #L15 - L16 were not covered by tests
30-32: Improved logging configurationUsing
logger = logging.getLogger(__name__)is a best practice that improves modularity by using the module's name for logging.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 31-32: cratedb_toolkit/docs/settings.py#L31-L32
Added lines #L31 - L32 were not covered by tests
53-58: Enhanced error handling and progress reportingThe improved progress reporting and explicit error handling for HTTP responses makes the code more robust.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 53-58: cratedb_toolkit/docs/settings.py#L53-L58
Added lines #L53 - L58 were not covered by tests
427-472: Refactored markdown table generation to return a stringThe function now correctly returns a string instead of writing directly to a file, which improves separation of concerns and flexibility in the API.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 427-427: cratedb_toolkit/docs/settings.py#L427
Added line #L427 was not covered by tests
[warning] 434-435: cratedb_toolkit/docs/settings.py#L434-L435
Added lines #L434 - L435 were not covered by tests
[warning] 437-438: cratedb_toolkit/docs/settings.py#L437-L438
Added lines #L437 - L438 were not covered by tests
[warning] 441-445: cratedb_toolkit/docs/settings.py#L441-L445
Added lines #L441 - L445 were not covered by tests
[warning] 448-448: cratedb_toolkit/docs/settings.py#L448
Added line #L448 was not covered by tests
[warning] 450-453: cratedb_toolkit/docs/settings.py#L450-L453
Added lines #L450 - L453 were not covered by tests
[warning] 455-455: cratedb_toolkit/docs/settings.py#L455
Added line #L455 was not covered by tests
[warning] 458-462: cratedb_toolkit/docs/settings.py#L458-L462
Added lines #L458 - L462 were not covered by tests
[warning] 464-464: cratedb_toolkit/docs/settings.py#L464
Added line #L464 was not covered by tests
[warning] 466-468: cratedb_toolkit/docs/settings.py#L466-L468
Added lines #L466 - L468 were not covered by tests
[warning] 470-470: cratedb_toolkit/docs/settings.py#L470
Added line #L470 was not covered by tests
[warning] 472-472: cratedb_toolkit/docs/settings.py#L472
Added line #L472 was not covered by tests
591-609: Well-implemented SQL statement writerThe new function generates SQL statements in a clean, formatted way and returns a string, consistent with the overall design improvements.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 591-594: cratedb_toolkit/docs/settings.py#L591-L594
Added lines #L591 - L594 were not covered by tests
[warning] 596-599: cratedb_toolkit/docs/settings.py#L596-L599
Added lines #L596 - L599 were not covered by tests
[warning] 602-603: cratedb_toolkit/docs/settings.py#L602-L603
Added lines #L602 - L603 were not covered by tests
[warning] 605-606: cratedb_toolkit/docs/settings.py#L605-L606
Added lines #L605 - L606 were not covered by tests
[warning] 608-609: cratedb_toolkit/docs/settings.py#L608-L609
Added lines #L608 - L609 were not covered by tests
612-659: Great OOP design with the SettingsExtractor classThe introduction of this class with a fluent interface pattern (method chaining) significantly improves the code organization and usability. The class encapsulates extraction, rendering, and output writing in a clear and maintainable way.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 612-613: cratedb_toolkit/docs/settings.py#L612-L613
Added lines #L612 - L613 were not covered by tests
[warning] 619-620: cratedb_toolkit/docs/settings.py#L619-L620
Added lines #L619 - L620 were not covered by tests
[warning] 622-622: cratedb_toolkit/docs/settings.py#L622
Added line #L622 was not covered by tests
[warning] 624-624: cratedb_toolkit/docs/settings.py#L624
Added line #L624 was not covered by tests
| def write_sql_statements(settings) -> str: | ||
| f = io.StringIO() | ||
| f.write("-- CrateDB Runtime Configurable Settings\n") | ||
| f.write("-- Generated by settings extractor\n\n") | ||
|
|
||
| This tool scrapes the CrateDB documentation to extract configuration settings, | ||
| their default values, descriptions, and runtime configurability status. | ||
| count = 0 | ||
| for _, setting_info in sorted(settings.items()): | ||
| if setting_info["runtime_configurable"] and "stmt" in setting_info: | ||
| stmt = setting_info["stmt"] | ||
|
|
||
| Examples: | ||
| # Ensure statement ends with semicolon | ||
| if not stmt.endswith(";"): | ||
| stmt += ";" | ||
|
|
||
| # Extract settings to JSON (default) | ||
| python soup2.py | ||
| f.write(f"{stmt}\n") | ||
| count += 1 | ||
|
|
||
| # Extract settings to Markdown | ||
| python soup2.py --format markdown | ||
| f.write(f"\n-- Total statements: {count}\n") | ||
| return f.getvalue() |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Improve test coverage for new functionality
The static analysis hints indicate that many of the changes in this file are not covered by tests. Consider adding more unit tests to ensure the new functionality works as expected, especially for the SettingsExtractor class methods.
🏁 Script executed:
#!/bin/bash
# Look for existing test coverage of SettingsExtractor class
echo "Checking for existing tests of SettingsExtractor class..."
rg "SettingsExtractor" tests/Length of output: 154
Action Required: Enhance Test Coverage for SettingsExtractor Functionality
- The current test suite does not appear to include any tests for the
SettingsExtractorclass methods, as confirmed by the shell script search. - Please add comprehensive unit tests to validate:
- The behavior of methods within the
SettingsExtractorclass. - Specific logic in the
write_sql_statementsfunction (e.g., ensuring statements end with a semicolon and counting the generated statements correctly).
- The behavior of methods within the
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 591-594: cratedb_toolkit/docs/settings.py#L591-L594
Added lines #L591 - L594 were not covered by tests
[warning] 596-599: cratedb_toolkit/docs/settings.py#L596-L599
Added lines #L596 - L599 were not covered by tests
[warning] 602-603: cratedb_toolkit/docs/settings.py#L602-L603
Added lines #L602 - L603 were not covered by tests
[warning] 605-606: cratedb_toolkit/docs/settings.py#L605-L606
Added lines #L605 - L606 were not covered by tests
[warning] 608-609: cratedb_toolkit/docs/settings.py#L608-L609
Added lines #L608 - L609 were not covered by tests
There was a problem hiding this comment.
@coderabbitai: That can be done in a subsequent iteration. Currently, it is good enough to validate the code by invoking the CLI subcommands, and validate that the output is reasonably correct.
There was a problem hiding this comment.
@amotl I understand your approach. Using CLI subcommand tests to validate the functionality is a practical solution for the current iteration, especially since they verify the actual output is correct. More granular unit tests for the SettingsExtractor class can be added in a subsequent PR when needed.
( \
\ \
\ \
\ \
\ \
\ \
/ \
/ \
/_ _\
(•) (•)
\ /
/ \
/ \
/ \
/__ ___\
/ \
\__/ thoughtful bunny
✏️ Learnings added
Learnt from: amotl
PR: crate/cratedb-toolkit#400
File: cratedb_toolkit/docs/settings.py:0-0
Timestamp: 2025-04-13T14:43:29.811Z
Learning: For the cratedb-toolkit project, functional validation through CLI commands is preferred as the initial testing approach, with more granular unit tests being deferred to subsequent iterations when necessary.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
amotl
force-pushed
the
docs-settings-stdout
branch
from
79a90aa to
238c917
Compare
1 participant
About
Streamline and trim the rendering and output writing subsystem a bit.
When no output parameter is given, emit results to stdout.
Also enable rendering into YAML format.
Python API