Conversation
WalkthroughThis pull request updates the Docs API documentation and the CLI tool to support extraction of both CrateDB functions and settings. It introduces new files for handling function extraction, output formatting, and processing, while modifying existing modules to streamline the extraction logic. The documentation now includes a section on CrateDB functions, and new tests validate the CLI commands for different output formats. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant CLI as CLI Module
participant FE as FunctionsExtractor
participant FR as FunctionRegistry
participant FF as FlexibleFormatter
participant O as Output Handler
U->>CLI: Run "ctk docs functions --format markdown --output file"
CLI->>FE: Invoke function extraction
FE->>FE: Fetch & parse CrateDB documentation
FE->>FR: Register functions
FR-->>FE: Return structured data
FE->>FF: Format data per specified output
FF-->>FE: Return formatted content
FE->>O: Write output to file/console
O-->>U: Deliver output/success status
Poem
✨ 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: 4
🔭 Outside diff range comments (1)
cratedb_toolkit/docs/functions.py (1)
1-101: 🛠️ Refactor suggestionAdd unit test coverage for new extractor functionality
Static analysis indicates that these lines lack test coverage. Consider adding tests to ensure functionality is verified and future regressions are prevented.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 1-5: cratedb_toolkit/docs/functions.py#L1-L5
Added lines #L1 - L5 were not covered by tests
[warning] 7-13: cratedb_toolkit/docs/functions.py#L7-L13
Added lines #L7 - L13 were not covered by tests
[warning] 15-15: cratedb_toolkit/docs/functions.py#L15
Added line #L15 was not covered by tests
[warning] 17-17: cratedb_toolkit/docs/functions.py#L17
Added line #L17 was not covered by tests
[warning] 20-20: cratedb_toolkit/docs/functions.py#L20
Added line #L20 was not covered by tests
[warning] 23-28: cratedb_toolkit/docs/functions.py#L23-L28
Added lines #L23 - L28 were not covered by tests
[warning] 30-31: cratedb_toolkit/docs/functions.py#L30-L31
Added lines #L30 - L31 were not covered by tests
[warning] 33-34: cratedb_toolkit/docs/functions.py#L33-L34
Added lines #L33 - L34 were not covered by tests
[warning] 37-40: cratedb_toolkit/docs/functions.py#L37-L40
Added lines #L37 - L40 were not covered by tests
[warning] 42-45: cratedb_toolkit/docs/functions.py#L42-L45
Added lines #L42 - L45 were not covered by tests
[warning] 47-48: cratedb_toolkit/docs/functions.py#L47-L48
Added lines #L47 - L48 were not covered by tests
[warning] 51-57: cratedb_toolkit/docs/functions.py#L51-L57
Added lines #L51 - L57 were not covered by tests
[warning] 60-61: cratedb_toolkit/docs/functions.py#L60-L61
Added lines #L60 - L61 were not covered by tests
[warning] 67-69: cratedb_toolkit/docs/functions.py#L67-L69
Added lines #L67 - L69 were not covered by tests
[warning] 71-74: cratedb_toolkit/docs/functions.py#L71-L74
Added lines #L71 - L74 were not covered by tests
[warning] 76-77: cratedb_toolkit/docs/functions.py#L76-L77
Added lines #L76 - L77 were not covered by tests
[warning] 81-88: cratedb_toolkit/docs/functions.py#L81-L88
Added lines #L81 - L88 were not covered by tests
[warning] 94-94: cratedb_toolkit/docs/functions.py#L94
Added line #L94 was not covered by tests
[warning] 96-100: cratedb_toolkit/docs/functions.py#L96-L100
Added lines #L96 - L100 were not covered by tests
🧹 Nitpick comments (7)
tests/docs/test_cli.py (1)
96-135: Good test coverage for new functionalityThe new test functions verify both JSON and Markdown output formats, following the same pattern as the existing tests for settings. They properly check for successful execution and expected content.
Consider adding a test for the YAML output format as well, since the CLI supports this format. Example:
def test_functions_yaml(tmp_path: Path): """ Verify `ctk docs functions` with YAML output. """ output_path = tmp_path / "cratedb-functions.yaml" # Invoke command. runner = CliRunner() result = runner.invoke( cli, args=f"functions --format=yaml --output={output_path}", catch_exceptions=False, ) assert result.exit_code == 0 # Verify the outcome. data = yaml.safe_load(output_path.read_text()) assert "substr('string' FROM 'pattern')" in data["functions"]cratedb_toolkit/docs/util.py (2)
10-13: Reword the docstring to reflect more generic usage
Since this class is namedGenericProcessor, updating the docstring to mention extracting "CrateDB artifacts" instead of just "settings" would better align its name and possible use cases.Apply this diff to broaden the docstring:
- Extract CrateDB settings from documentation. + Extract CrateDB artifacts (e.g., settings, functions) from documentation.
26-34: Consider handling file I/O errors
When writing the payload to a file, issues like permission errors or missing paths may arise. Handling or logging exceptions can improve reliability.cratedb_toolkit/docs/settings.py (4)
617-618: Implementingto_markdown()method.Using
write_markdown_table()ensures consistent handling with existing logic. Add tests to confirm correct formatting of sample input data, especially corner cases (e.g., empty or large sets of settings).🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 617-618: cratedb_toolkit/docs/settings.py#L617-L618
Added lines #L617 - L618 were not covered by tests
620-621: Implementingto_sql()method.This method wraps
write_sql_statements()neatly. Like above, test coverage is recommended; verifying generated SQL is correct helps avoid runtime issues.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 620-621: cratedb_toolkit/docs/settings.py#L620-L621
Added lines #L620 - L621 were not covered by tests
639-639: Error handling for empty extraction.Exiting the script if no settings are extracted is appropriate here. If this functionality must run in non-interactive contexts, consider raising an exception or returning a status code for better integration.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 639-639: cratedb_toolkit/docs/settings.py#L639
Added line #L639 was not covered by tests
648-648: Delegating to the superclass render method.Calling
super().render(format_)ensures that theGenericProcessorlogic remains central. Testing custom format flows (JSON, YAML, Markdown, SQL) will help maintain coverage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
CHANGES.md(1 hunks)cratedb_toolkit/docs/cli.py(2 hunks)cratedb_toolkit/docs/functions.py(1 hunks)cratedb_toolkit/docs/settings.py(2 hunks)cratedb_toolkit/docs/util.py(1 hunks)cratedb_toolkit/util/format.py(1 hunks)doc/docs-api.md(1 hunks)tests/docs/test_cli.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
cratedb_toolkit/docs/cli.py (3)
cratedb_toolkit/util/cli.py (1)
make_command(75-89)cratedb_toolkit/docs/functions.py (2)
FunctionsExtractor(61-100)acquire(71-100)cratedb_toolkit/docs/util.py (1)
write(26-34)
cratedb_toolkit/docs/functions.py (2)
cratedb_toolkit/docs/util.py (1)
GenericProcessor(9-34)cratedb_toolkit/docs/cli.py (1)
functions(71-80)
cratedb_toolkit/docs/settings.py (2)
cratedb_toolkit/docs/util.py (2)
GenericProcessor(9-34)render(19-24)cratedb_toolkit/util/format.py (5)
format(24-44)FlexibleFormatter(21-64)OutputFormat(11-17)to_markdown(55-61)to_sql(63-64)
🪛 GitHub Check: codecov/patch
cratedb_toolkit/docs/cli.py
[warning] 77-77: cratedb_toolkit/docs/cli.py#L77
Added line #L77 was not covered by tests
[warning] 79-80: cratedb_toolkit/docs/cli.py#L79-L80
Added lines #L79 - L80 were not covered by tests
cratedb_toolkit/docs/functions.py
[warning] 1-5: cratedb_toolkit/docs/functions.py#L1-L5
Added lines #L1 - L5 were not covered by tests
[warning] 7-13: cratedb_toolkit/docs/functions.py#L7-L13
Added lines #L7 - L13 were not covered by tests
[warning] 15-15: cratedb_toolkit/docs/functions.py#L15
Added line #L15 was not covered by tests
[warning] 17-17: cratedb_toolkit/docs/functions.py#L17
Added line #L17 was not covered by tests
[warning] 20-20: cratedb_toolkit/docs/functions.py#L20
Added line #L20 was not covered by tests
[warning] 23-28: cratedb_toolkit/docs/functions.py#L23-L28
Added lines #L23 - L28 were not covered by tests
[warning] 30-31: cratedb_toolkit/docs/functions.py#L30-L31
Added lines #L30 - L31 were not covered by tests
[warning] 33-34: cratedb_toolkit/docs/functions.py#L33-L34
Added lines #L33 - L34 were not covered by tests
[warning] 37-40: cratedb_toolkit/docs/functions.py#L37-L40
Added lines #L37 - L40 were not covered by tests
[warning] 42-45: cratedb_toolkit/docs/functions.py#L42-L45
Added lines #L42 - L45 were not covered by tests
[warning] 47-48: cratedb_toolkit/docs/functions.py#L47-L48
Added lines #L47 - L48 were not covered by tests
[warning] 51-57: cratedb_toolkit/docs/functions.py#L51-L57
Added lines #L51 - L57 were not covered by tests
[warning] 60-61: cratedb_toolkit/docs/functions.py#L60-L61
Added lines #L60 - L61 were not covered by tests
[warning] 67-69: cratedb_toolkit/docs/functions.py#L67-L69
Added lines #L67 - L69 were not covered by tests
[warning] 71-74: cratedb_toolkit/docs/functions.py#L71-L74
Added lines #L71 - L74 were not covered by tests
[warning] 76-77: cratedb_toolkit/docs/functions.py#L76-L77
Added lines #L76 - L77 were not covered by tests
[warning] 81-88: cratedb_toolkit/docs/functions.py#L81-L88
Added lines #L81 - L88 were not covered by tests
[warning] 94-94: cratedb_toolkit/docs/functions.py#L94
Added line #L94 was not covered by tests
[warning] 96-100: cratedb_toolkit/docs/functions.py#L96-L100
Added lines #L96 - L100 were not covered by tests
cratedb_toolkit/docs/settings.py
[warning] 20-20: cratedb_toolkit/docs/settings.py#L20
Added line #L20 was not covered by tests
[warning] 27-28: cratedb_toolkit/docs/settings.py#L27-L28
Added lines #L27 - L28 were not covered by tests
[warning] 612-612: cratedb_toolkit/docs/settings.py#L612
Added line #L612 was not covered by tests
[warning] 617-618: cratedb_toolkit/docs/settings.py#L617-L618
Added lines #L617 - L618 were not covered by tests
[warning] 620-621: cratedb_toolkit/docs/settings.py#L620-L621
Added lines #L620 - L621 were not covered by tests
[warning] 625-625: cratedb_toolkit/docs/settings.py#L625
Added line #L625 was not covered by tests
[warning] 631-632: cratedb_toolkit/docs/settings.py#L631-L632
Added lines #L631 - L632 were not covered by tests
[warning] 637-637: cratedb_toolkit/docs/settings.py#L637
Added line #L637 was not covered by tests
[warning] 639-639: cratedb_toolkit/docs/settings.py#L639
Added line #L639 was not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: build-and-publish
- GitHub Check: Kinesis: Python 3.12 on OS ubuntu-latest
- GitHub Check: Kinesis: Python 3.9 on OS ubuntu-latest
- GitHub Check: Generic: Python 3.12 on OS ubuntu-latest
- GitHub Check: Generic: Python 3.9 on OS ubuntu-latest
- GitHub Check: Generic: Python 3.8 on OS ubuntu-latest
- GitHub Check: CFR for OS windows-latest
- GitHub Check: CFR for OS ubuntu-latest
- GitHub Check: CFR for OS macos-13
🔇 Additional comments (12)
CHANGES.md (1)
6-6: Accurate changelog updateThe update to the changelog entry correctly reflects the added functionality for extracting both CrateDB functions and settings, providing clear documentation of the new feature.
doc/docs-api.md (1)
12-51: Well-structured documentation additionThe new section on CrateDB functions is comprehensive and follows the same format as the existing settings section. The examples are clear and practical, showing both the command usage and sample output in JSON format.
cratedb_toolkit/docs/cli.py (3)
22-37: Well-documented help functionThe
help_functionsdocstring follows the established pattern in the file, providing clear examples of command usage for different formats and output options.
40-43: Improved clarity in help textThe modified description in
help_settingsis now more specific about extracting "configuration settings," which improves clarity.
57-57: Consistent naming conventionUpdated the example output filename to follow the consistent naming pattern with other examples (
cratedb-settings.md).cratedb_toolkit/docs/util.py (1)
19-25:render()implementation looks correct
Uses theformatterproperly to convert data to the requested format. No issues found here.cratedb_toolkit/docs/settings.py (6)
20-20: Import addition looks good.This import is necessary to support the introduced type annotations (
Type[FlexibleFormatter], etc.).🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 20-20: cratedb_toolkit/docs/settings.py#L20
Added line #L20 was not covered by tests
27-28: Imports forGenericProcessorand formatting classes.These imports align with the new inheritance structure in this file. Just ensure that references to
GenericProcessorandFlexibleFormatterremain consistent across the codebase.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 27-28: cratedb_toolkit/docs/settings.py#L27-L28
Added lines #L27 - L28 were not covered by tests
631-632:thingattribute type and the defaultformatter.Renaming
settingstothingfor consistency withGenericProcessoris fine—just ensure references are updated across the codebase. Also, defaulting toOutputFormatteris convenient.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 631-632: cratedb_toolkit/docs/settings.py#L631-L632
Added lines #L631 - L632 were not covered by tests
637-637: Acquiring settings data.Assigning extracted settings into
self.thingis straightforward. Consider verifying the extracted data to confirm correctness (e.g., ensuring no unexpected empty fields).🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 637-637: cratedb_toolkit/docs/settings.py#L637
Added line #L637 was not covered by tests
644-644: Generating SQL statements after data extraction.This call to
generate_sql_statements(self.thing)is well-timed. Confirm that no unexpected transformations happen toself.thingthat might conflict with a subsequent format rendering.
651-652: Logging the summary of runtime-configurable settings.This log line is helpful for diagnosing coverage of runtime settings. Consider adding a test that inspects this value for correctness in both typical and edge cases (such as zero runtime-configurable settings).
amotl
force-pushed
the
docs-api-functions
branch
2 times, most recently
from
bceaefc to
44f447f
Compare
Docstrings generation was requested by @amotl. * #401 (comment) The following files were modified: * `cratedb_toolkit/docs/cli.py` * `cratedb_toolkit/docs/functions.py` * `cratedb_toolkit/docs/settings.py` * `cratedb_toolkit/docs/util.py` * `cratedb_toolkit/util/format.py`
|
Note Generated docstrings for this pull request at #402 |
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
cratedb_toolkit/docs/settings.py (1)
1-654: 💡 Verification agent🧩 Analysis chain
Test coverage concerns for refactored code.
Several modified lines have been flagged by static analysis as not covered by tests, including the new OutputFormatter class and changes to SettingsExtractor. Consider adding tests to validate the behavior of these components.
Let's check existing test coverage for the settings module:
🏁 Script executed:
#!/bin/bash # Search for existing tests for the settings module rg -A 3 "test.*settings" --type=py rg -A 5 "SettingsExtractor.*test" --type=pyLength of output: 2719
Action Required: Enhance Test Coverage for Settings Extraction
The static analysis has flagged that the new
OutputFormatterclass and recent modifications toSettingsExtractorare not covered by dedicated tests. Although there are some tests undertests/docs/that indirectly exercise settings functionality (e.g., for JSON, Markdown, SQL, and YAML output), a focused test suite for these new components appears to be missing.
- Ensure unit tests are added for:
- The new
OutputFormatterclass to verify that theto_markdownandto_sqlmethods produce the expected outputs.- Modifications in
SettingsExtractor(especially itsacquireandrendermethods) to validate proper settings extraction and SQL statement generation.- Confirm that any further refactoring in these areas is accompanied by tests to safeguard against regressions.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 20-20: cratedb_toolkit/docs/settings.py#L20
Added line #L20 was not covered by tests
[warning] 27-28: cratedb_toolkit/docs/settings.py#L27-L28
Added lines #L27 - L28 were not covered by tests
[warning] 612-612: cratedb_toolkit/docs/settings.py#L612
Added line #L612 was not covered by tests
[warning] 617-618: cratedb_toolkit/docs/settings.py#L617-L618
Added lines #L617 - L618 were not covered by tests
[warning] 620-621: cratedb_toolkit/docs/settings.py#L620-L621
Added lines #L620 - L621 were not covered by tests
[warning] 625-625: cratedb_toolkit/docs/settings.py#L625
Added line #L625 was not covered by tests
[warning] 631-632: cratedb_toolkit/docs/settings.py#L631-L632
Added lines #L631 - L632 were not covered by tests
[warning] 637-637: cratedb_toolkit/docs/settings.py#L637
Added line #L637 was not covered by tests
[warning] 639-639: cratedb_toolkit/docs/settings.py#L639
Added line #L639 was not covered by tests
♻️ Duplicate comments (1)
cratedb_toolkit/docs/settings.py (1)
612-621: OutputFormatter class creation.Defining a dedicated
OutputFormattersubclass is a clean approach for specialized rendering to Markdown or SQL. Be mindful of test coverage, as static analysis indicates these lines are not tested.Let's check if there are tests for the new formatter class:
#!/bin/bash # Search for tests covering the OutputFormatter class rg -A 5 "test.*OutputFormatter" --type=py rg -A 5 "OutputFormatter.*test" --type=py🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 612-612: cratedb_toolkit/docs/settings.py#L612
Added line #L612 was not covered by tests
[warning] 617-618: cratedb_toolkit/docs/settings.py#L617-L618
Added lines #L617 - L618 were not covered by tests
[warning] 620-621: cratedb_toolkit/docs/settings.py#L620-L621
Added lines #L620 - L621 were not covered by tests
🧹 Nitpick comments (9)
cratedb_toolkit/docs/util.py (3)
16-16: Clarify default value usage forformatter.
The field is defined as a class rather than an instance. While intentional, it might be confusing because a newFlexibleFormatterinstance is created at runtime withself.formatter(self.thing). Consider adding a comment to clarify the design.
19-24: Add a test case for invalid formats.
The code raises aValueErrorif an unsupported format is specified. To ensure coverage and prevent regressions, consider adding a test that attempts to render an invalid format like"xyz".
26-34: Consider returning or logging the write destination.
When writing to a file, returning or logging the output path can improve traceability. This is optional, but can be helpful for troubleshooting.cratedb_toolkit/docs/cli.py (1)
22-37: Correct the docstring examples for thefunctionshelp command.
Examples currently reference 'settings' instead of 'functions'. This can cause confusion among users.- # Extract settings to JSON (default) - ctk docs functions ... - # Extract settings to Markdown - ctk docs functions --format markdown ... - # Specify custom output file - ctk docs settings --format markdown --output cratedb-functions.md + # Extract functions to JSON (default) + ctk docs functions ... + # Extract functions to Markdown + ctk docs functions --format markdown ... + # Specify custom output file + ctk docs functions --format markdown --output cratedb-functions.mdcratedb_toolkit/util/format.py (2)
11-17: Update docstring to reflect broader usage.
This enum is used not just bySettingsExtractorbut also byFunctionsExtractor. Consider renaming or expanding the docstring.
20-65: Assess specialized handling of dictionary-based data.
to_markdownassumesself.thingis a dictionary of nested dictionaries. If used with different data shapes, this might break. Consider introducing more specialized formatters or gracefully handling alternate structures.cratedb_toolkit/docs/functions.py (2)
20-20: Consider making the documentation URL configurable.
Referencing a fixed branch (5.10) from GitHub might break or become outdated. In a long-lived system, offering a configurable URL or referencing a stable branch can increase maintainability.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 20-20: cratedb_toolkit/docs/functions.py#L20
Added line #L20 was not covered by tests
29-29: Provide a plan for the TODO.
The comment indicates a future step to parsereturnsandexamplefrom the description. If this feature is essential, consider implementing it soon or creating a follow-up task.cratedb_toolkit/docs/settings.py (1)
625-625: SettingsExtractor refactored to use GenericProcessor.Good architecture decision to inherit from GenericProcessor for code reuse. The attribute rename from
settingstothingenhances consistency with the base class, and adding theformatterattribute with a default value improves flexibility.Consider adding a more descriptive type hint for
thingto improve code readability:- thing: Dict[str, Dict[str, Any]] = dataclasses.field(default_factory=dict) + thing: Dict[str, Dict[str, Any]] = dataclasses.field(default_factory=dict, metadata={"description": "Dictionary of CrateDB settings"})Also applies to: 631-633
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 625-625: cratedb_toolkit/docs/settings.py#L625
Added line #L625 was not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
CHANGES.md(1 hunks)cratedb_toolkit/docs/cli.py(2 hunks)cratedb_toolkit/docs/functions.py(1 hunks)cratedb_toolkit/docs/settings.py(2 hunks)cratedb_toolkit/docs/util.py(1 hunks)cratedb_toolkit/util/format.py(1 hunks)doc/docs-api.md(1 hunks)tests/docs/test_cli.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- doc/docs-api.md
- CHANGES.md
- tests/docs/test_cli.py
🧰 Additional context used
🧬 Code Graph Analysis (5)
cratedb_toolkit/docs/util.py (1)
cratedb_toolkit/util/format.py (3)
format(24-44)FlexibleFormatter(21-64)OutputFormat(11-17)
cratedb_toolkit/util/format.py (3)
cratedb_toolkit/util/data_dict.py (2)
copy(164-165)items(161-162)cratedb_toolkit/docs/settings.py (2)
to_markdown(617-618)to_sql(620-621)cratedb_toolkit/docs/util.py (1)
write(26-34)
cratedb_toolkit/docs/cli.py (1)
cratedb_toolkit/docs/functions.py (2)
FunctionsExtractor(61-100)acquire(71-100)
cratedb_toolkit/docs/settings.py (2)
cratedb_toolkit/docs/util.py (2)
GenericProcessor(9-34)render(19-24)cratedb_toolkit/util/format.py (5)
format(24-44)FlexibleFormatter(21-64)OutputFormat(11-17)to_markdown(55-61)to_sql(63-64)
cratedb_toolkit/docs/functions.py (2)
cratedb_toolkit/docs/util.py (1)
GenericProcessor(9-34)cratedb_toolkit/docs/cli.py (1)
functions(71-80)
🪛 GitHub Check: codecov/patch
cratedb_toolkit/docs/cli.py
[warning] 77-77: cratedb_toolkit/docs/cli.py#L77
Added line #L77 was not covered by tests
[warning] 79-80: cratedb_toolkit/docs/cli.py#L79-L80
Added lines #L79 - L80 were not covered by tests
cratedb_toolkit/docs/settings.py
[warning] 20-20: cratedb_toolkit/docs/settings.py#L20
Added line #L20 was not covered by tests
[warning] 27-28: cratedb_toolkit/docs/settings.py#L27-L28
Added lines #L27 - L28 were not covered by tests
[warning] 612-612: cratedb_toolkit/docs/settings.py#L612
Added line #L612 was not covered by tests
[warning] 617-618: cratedb_toolkit/docs/settings.py#L617-L618
Added lines #L617 - L618 were not covered by tests
[warning] 620-621: cratedb_toolkit/docs/settings.py#L620-L621
Added lines #L620 - L621 were not covered by tests
[warning] 625-625: cratedb_toolkit/docs/settings.py#L625
Added line #L625 was not covered by tests
[warning] 631-632: cratedb_toolkit/docs/settings.py#L631-L632
Added lines #L631 - L632 were not covered by tests
[warning] 637-637: cratedb_toolkit/docs/settings.py#L637
Added line #L637 was not covered by tests
[warning] 639-639: cratedb_toolkit/docs/settings.py#L639
Added line #L639 was not covered by tests
cratedb_toolkit/docs/functions.py
[warning] 1-5: cratedb_toolkit/docs/functions.py#L1-L5
Added lines #L1 - L5 were not covered by tests
[warning] 7-13: cratedb_toolkit/docs/functions.py#L7-L13
Added lines #L7 - L13 were not covered by tests
[warning] 15-15: cratedb_toolkit/docs/functions.py#L15
Added line #L15 was not covered by tests
[warning] 17-17: cratedb_toolkit/docs/functions.py#L17
Added line #L17 was not covered by tests
[warning] 20-20: cratedb_toolkit/docs/functions.py#L20
Added line #L20 was not covered by tests
[warning] 23-28: cratedb_toolkit/docs/functions.py#L23-L28
Added lines #L23 - L28 were not covered by tests
[warning] 30-31: cratedb_toolkit/docs/functions.py#L30-L31
Added lines #L30 - L31 were not covered by tests
[warning] 33-34: cratedb_toolkit/docs/functions.py#L33-L34
Added lines #L33 - L34 were not covered by tests
[warning] 37-40: cratedb_toolkit/docs/functions.py#L37-L40
Added lines #L37 - L40 were not covered by tests
[warning] 42-45: cratedb_toolkit/docs/functions.py#L42-L45
Added lines #L42 - L45 were not covered by tests
[warning] 47-48: cratedb_toolkit/docs/functions.py#L47-L48
Added lines #L47 - L48 were not covered by tests
[warning] 51-57: cratedb_toolkit/docs/functions.py#L51-L57
Added lines #L51 - L57 were not covered by tests
[warning] 60-61: cratedb_toolkit/docs/functions.py#L60-L61
Added lines #L60 - L61 were not covered by tests
[warning] 67-69: cratedb_toolkit/docs/functions.py#L67-L69
Added lines #L67 - L69 were not covered by tests
[warning] 71-74: cratedb_toolkit/docs/functions.py#L71-L74
Added lines #L71 - L74 were not covered by tests
[warning] 76-77: cratedb_toolkit/docs/functions.py#L76-L77
Added lines #L76 - L77 were not covered by tests
[warning] 81-88: cratedb_toolkit/docs/functions.py#L81-L88
Added lines #L81 - L88 were not covered by tests
[warning] 94-94: cratedb_toolkit/docs/functions.py#L94
Added line #L94 was not covered by tests
[warning] 96-100: cratedb_toolkit/docs/functions.py#L96-L100
Added lines #L96 - L100 were not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: build-and-publish
- GitHub Check: Generic: Python 3.12 on OS ubuntu-latest
- GitHub Check: Generic: Python 3.9 on OS ubuntu-latest
- GitHub Check: Generic: Python 3.8 on OS ubuntu-latest
- GitHub Check: CFR for OS windows-latest
- GitHub Check: CFR for OS macos-13
🔇 Additional comments (5)
cratedb_toolkit/docs/cli.py (1)
61-81: Add dedicated tests for the newfunctionscommand.
Lines 77-80 are not covered. Consider adding direct tests for.acquire().render(format_).write(output), ensuring that various formats are handled correctly.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 77-77: cratedb_toolkit/docs/cli.py#L77
Added line #L77 was not covered by tests
[warning] 79-80: cratedb_toolkit/docs/cli.py#L79-L80
Added lines #L79 - L80 were not covered by testscratedb_toolkit/docs/settings.py (4)
637-640: Update to use 'thing' attribute.This change correctly updates the variable naming to align with the GenericProcessor base class.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 637-637: cratedb_toolkit/docs/settings.py#L637
Added line #L637 was not covered by tests
[warning] 639-639: cratedb_toolkit/docs/settings.py#L639
Added line #L639 was not covered by tests
648-649: Simplified render method.Excellent simplification by delegating to the superclass implementation. This reduces code duplication and leverages the common functionality in GenericProcessor.
651-653: Updated runtime count calculation.The runtime count calculation has been updated to use the new
thingattribute, maintaining functionality while adapting to the new structure.
20-20:Details
✅ Verification successful
Import structure updates align with new architecture.
The imports have been updated to support the new class inheritance structure, with
GenericProcessorandFlexibleFormatternow being imported as part of the refactoring.Let's verify the import structure is consistent across related files:
Also applies to: 27-28
🏁 Script executed:
#!/bin/bash # Check imports of GenericProcessor and FlexibleFormatter in related files rg -A 3 "import.*GenericProcessor" --type=py rg -A 3 "import.*FlexibleFormatter" --type=pyLength of output: 1087
Verified Import Structure – Consistent Across Files
The refactoring updates for importing
GenericProcessorandFlexibleFormatterhave been verified across the relevant files:
- cratedb_toolkit/docs/settings.py imports
GenericProcessorfromcratedb_toolkit/docs/utilandFlexibleFormatter, OutputFormatfromcratedb_toolkit/util.format, matching the new inheritance structure.- cratedb_toolkit/docs/functions.py similarly imports
GenericProcessorfromcratedb_toolkit/docs/util.- cratedb_toolkit/docs/util.py confirms the import of
FlexibleFormatterconsistent with the intended update.These changes ensure that both the settings on lines 20 and 27–28 and their usage in related files are aligned with the new architecture.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 20-20: cratedb_toolkit/docs/settings.py#L20
Added line #L20 was not covered by tests
amotl
force-pushed
the
docs-api-functions
branch
2 times, most recently
from
cd71083 to
f8f4803
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (2)
cratedb_toolkit/docs/functions.py (1)
1-133: 💡 Verification agent🧩 Analysis chain
Add comprehensive test coverage for the FunctionsExtractor.
Static analysis shows that most lines in this file lack test coverage. Consider adding unit tests to ensure the functionality works as expected, especially for error handling and edge cases.
🏁 Script executed:
#!/bin/bash # Description: Check if any tests exist for functions.py # Search for tests related to functions.py echo "Looking for existing tests for functions.py..." fd --type file --extension py "test.*function|function.*test" tests/ # Search for functions.py in test files that might import it echo "Looking for imports of functions.py in test files..." rg "from cratedb_toolkit.docs.functions import" --type py tests/Length of output: 355
Test Coverage for FunctionsExtractor is Missing – Please Add Unit Tests
It appears there are no tests covering
cratedb_toolkit/docs/functions.py, especially for theFunctionsExtractorclass. To ensure robust functionality, please add comprehensive unit tests. Consider the following:
- Normal Execution: Validate that functions are extracted correctly from valid documentation input.
- Error Handling: Verify that the extractor correctly logs an error or handles cases with no functions found.
- Edge Cases: Ensure proper behavior when duplicate function signatures are encountered (e.g., raising a
ValueError).🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 1-4: cratedb_toolkit/docs/functions.py#L1-L4
Added lines #L1 - L4 were not covered by tests
[warning] 6-12: cratedb_toolkit/docs/functions.py#L6-L12
Added lines #L6 - L12 were not covered by tests
[warning] 14-14: cratedb_toolkit/docs/functions.py#L14
Added line #L14 was not covered by tests
[warning] 16-16: cratedb_toolkit/docs/functions.py#L16
Added line #L16 was not covered by tests
[warning] 19-19: cratedb_toolkit/docs/functions.py#L19
Added line #L19 was not covered by tests
[warning] 22-27: cratedb_toolkit/docs/functions.py#L22-L27
Added lines #L22 - L27 were not covered by tests
[warning] 29-30: cratedb_toolkit/docs/functions.py#L29-L30
Added lines #L29 - L30 were not covered by tests
[warning] 32-32: cratedb_toolkit/docs/functions.py#L32
Added line #L32 was not covered by tests
[warning] 39-39: cratedb_toolkit/docs/functions.py#L39
Added line #L39 was not covered by tests
[warning] 42-45: cratedb_toolkit/docs/functions.py#L42-L45
Added lines #L42 - L45 were not covered by tests
[warning] 47-47: cratedb_toolkit/docs/functions.py#L47
Added line #L47 was not covered by tests
[warning] 57-59: cratedb_toolkit/docs/functions.py#L57-L59
Added lines #L57 - L59 were not covered by tests
[warning] 61-61: cratedb_toolkit/docs/functions.py#L61
Added line #L61 was not covered by tests
[warning] 68-68: cratedb_toolkit/docs/functions.py#L68
Added line #L68 was not covered by tests
[warning] 71-77: cratedb_toolkit/docs/functions.py#L71-L77
Added lines #L71 - L77 were not covered by tests
[warning] 80-81: cratedb_toolkit/docs/functions.py#L80-L81
Added lines #L80 - L81 were not covered by tests
[warning] 87-89: cratedb_toolkit/docs/functions.py#L87-L89
Added lines #L87 - L89 were not covered by tests
[warning] 91-91: cratedb_toolkit/docs/functions.py#L91
Added line #L91 was not covered by tests
[warning] 104-106: cratedb_toolkit/docs/functions.py#L104-L106
Added lines #L104 - L106 were not covered by tests
[warning] 108-109: cratedb_toolkit/docs/functions.py#L108-L109
Added lines #L108 - L109 were not covered by tests
[warning] 113-120: cratedb_toolkit/docs/functions.py#L113-L120
Added lines #L113 - L120 were not covered by tests
[warning] 126-126: cratedb_toolkit/docs/functions.py#L126
Added line #L126 was not covered by tests
[warning] 128-129: cratedb_toolkit/docs/functions.py#L128-L129
Added lines #L128 - L129 were not covered by tests
[warning] 131-132: cratedb_toolkit/docs/functions.py#L131-L132
Added lines #L131 - L132 were not covered by teststests/util/test_format.py (1)
1-22: 🛠️ Refactor suggestionAdd more comprehensive test coverage for FlexibleFormatter.
The tests only cover JSON formatting and error handling for unknown formats. Consider adding tests for YAML, Markdown, and SQL formats, as well as edge cases like empty data and complex structures.
def test_formatter_yaml(): formatter = FlexibleFormatter(source_data) yaml_data = yaml.dump(source_data, sort_keys=False) assert formatter.format("yaml") == yaml_data assert formatter.format(OutputFormat.YAML) == yaml_data def test_formatter_markdown(): formatter = FlexibleFormatter(source_data) expected_markdown = "## thing\n### 42.42\n```yaml\n42.42\n```\n\n" result = formatter.format("markdown") assert "## thing" in result assert "42.42" in result def test_formatter_not_implemented(): formatter = FlexibleFormatter(source_data) with pytest.raises(NotImplementedError) as ex: formatter.format("sql") assert ex.match("Formatting to SQL needs a domain-specific implementation") def test_formatter_with_empty_data(): formatter = FlexibleFormatter({}) assert formatter.format("json") == "{}" def test_formatter_non_dict_markdown(): formatter = FlexibleFormatter([1, 2, 3]) with pytest.raises(NotImplementedError) as ex: formatter.format("markdown") assert ex.match("Unable to convert to Markdown")
♻️ Duplicate comments (1)
cratedb_toolkit/docs/cli.py (1)
77-80: Add CLI tests for functions command or discuss with team.I notice that past reviews flagged the lack of test coverage for the functions command, but the response indicated that CLI tests were considered sufficient. Since CLI tests would likely exercise this code indirectly, this may be acceptable, but it's worth confirming.
#!/bin/bash # Description: Check for any CLI tests that might cover the functions command # Search for CLI tests fd -e py "cli_test|test_cli" tests/ # Search for specific test of functions command rg "functions.*--format" --type py tests/🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 77-77: cratedb_toolkit/docs/cli.py#L77
Added line #L77 was not covered by tests
[warning] 79-80: cratedb_toolkit/docs/cli.py#L79-L80
Added lines #L79 - L80 were not covered by tests
🧹 Nitpick comments (8)
cratedb_toolkit/docs/functions.py (3)
19-19: Consider making the documentation URL configurable.The URL is currently hardcoded to a specific version (5.10) of the CrateDB documentation. Making this configurable would allow extracting functions from different versions without changing the code.
-DOCS_URL = "https://github.com/crate/crate/raw/refs/heads/5.10/docs/general/builtins/scalar-functions.rst" +DOCS_URL = os.environ.get( + "CRATEDB_DOCS_URL", + "https://github.com/crate/crate/raw/refs/heads/5.10/docs/general/builtins/scalar-functions.rst" +)🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 19-19: cratedb_toolkit/docs/functions.py#L19
Added line #L19 was not covered by tests
104-106: Add error handling for Sphinx processing.The code registers Sphinx roles and directives without error handling. If there are issues with the document structure, the process might fail silently.
- register_canonical_role("ref", sphinx_ref_role) - register_directive("seealso", Note) + try: + register_canonical_role("ref", sphinx_ref_role) + register_directive("seealso", Note) + except Exception as e: + logger.warning(f"Failed to register Sphinx roles/directives: {e}")🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 104-106: cratedb_toolkit/docs/functions.py#L104-L106
Added lines #L104 - L106 were not covered by tests
113-127: Add more robust error handling for document parsing.The nested loops that extract functions don't handle edge cases like malformed documents or unexpected structure. Consider adding more error handling and validation.
for item in document: - if item.tagname == "section": + try: + if item.tagname == "section": + category_title = item.children[0].astext() + for function in item.children: # type: ignore[assignment] + if function.tagname == "section": + function_title = function.children[0].astext() + function_body = function.children[1].astext() + fun = Function( + name=function_title.split("(")[0], + signature=function_title, + category=category_title, + description=function_body, + ) + self.registry.register(fun) + except (AttributeError, IndexError) as e: + logger.warning(f"Error processing document section: {e}")🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 113-120: cratedb_toolkit/docs/functions.py#L113-L120
Added lines #L113 - L120 were not covered by tests
[warning] 126-126: cratedb_toolkit/docs/functions.py#L126
Added line #L126 was not covered by testscratedb_toolkit/util/format.py (3)
65-67: Consider using a safer approach for deepcopy.While deepcopy is generally fine, it can fail with certain objects that aren't cleanly copyable. Consider adding error handling or a fallback.
def to_dict(self) -> dict: - return deepcopy(self.thing) + try: + return deepcopy(self.thing) + except (TypeError, RecursionError) as e: + # Fallback to shallow copy or serialization/deserialization + return {k: v for k, v in self.thing.items()} if isinstance(self.thing, dict) else self.thing
74-86: Enhance Markdown formatting.The current Markdown implementation is basic and might not handle complex structures elegantly. Consider enhancing it to better format nested structures and various data types.
def to_markdown(self) -> str: if not isinstance(self.thing, dict): raise NotImplementedError(f"Unable to convert to Markdown: {self.thing}") buffer = io.StringIO() for section, content in self.thing.items(): - buffer.write(f"## {section}") + buffer.write(f"## {section}\n\n") if isinstance(content, dict): for key, value in content.items(): - buffer.write(f"### {key}\n```yaml\n{yaml.dump(value)}```\n\n") + buffer.write(f"### {key}\n\n```yaml\n{yaml.dump(value, sort_keys=False)}```\n\n") else: - buffer.write(str(content)) + buffer.write(f"{str(content)}\n\n") return buffer.getvalue()
87-88: Add a return type hint to to_sql method.The to_sql method is missing a return type annotation. Even though it currently raises NotImplementedError, it should have a proper type hint.
- def to_sql(self): + def to_sql(self) -> str: raise NotImplementedError("Formatting to SQL needs a domain-specific implementation")cratedb_toolkit/docs/settings.py (2)
590-605: Consider consolidating similar SQL generation routines.This function appears to share partial functionality with
print_sql_statements()andgenerate_sql_statements(), leading to slight duplication in how SQL statements are managed or presented. As a minor improvement for maintainability, consider factoring out the common logic into a shared utility method or unifying them for clarity.
626-647: Add targeted unit tests if increased coverage is desired.While the new
OutputFormattermethods should be sufficiently exercised by CLI integration tests, static analysis notes these lines are not covered. If strict coverage is a requirement, adding simple unit tests can concretely verify the Markdown and SQL output without relying solely on end-to-end CLI tests.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 626-626: cratedb_toolkit/docs/settings.py#L626
Added line #L626 was not covered by tests
[warning] 631-631: cratedb_toolkit/docs/settings.py#L631
Added line #L631 was not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
CHANGES.md(1 hunks)cratedb_toolkit/docs/cli.py(2 hunks)cratedb_toolkit/docs/functions.py(1 hunks)cratedb_toolkit/docs/settings.py(3 hunks)cratedb_toolkit/docs/util.py(1 hunks)cratedb_toolkit/util/format.py(1 hunks)doc/docs-api.md(1 hunks)tests/docs/test_cli.py(1 hunks)tests/util/test_format.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- CHANGES.md
- doc/docs-api.md
- tests/docs/test_cli.py
- cratedb_toolkit/docs/util.py
🧰 Additional context used
🧬 Code Graph Analysis (4)
tests/util/test_format.py (1)
cratedb_toolkit/util/format.py (3)
format(24-63)FlexibleFormatter(21-88)OutputFormat(11-17)
cratedb_toolkit/docs/cli.py (2)
cratedb_toolkit/util/cli.py (1)
make_command(75-89)cratedb_toolkit/docs/functions.py (2)
FunctionsExtractor(81-132)acquire(91-132)
cratedb_toolkit/util/format.py (1)
cratedb_toolkit/docs/settings.py (2)
to_markdown(631-638)to_sql(640-647)
cratedb_toolkit/docs/functions.py (3)
cratedb_toolkit/docs/util.py (1)
GenericProcessor(9-61)cratedb_toolkit/model.py (1)
asdict(134-135)cratedb_toolkit/docs/cli.py (1)
functions(71-80)
🪛 GitHub Check: codecov/patch
cratedb_toolkit/docs/cli.py
[warning] 77-77: cratedb_toolkit/docs/cli.py#L77
Added line #L77 was not covered by tests
[warning] 79-80: cratedb_toolkit/docs/cli.py#L79-L80
Added lines #L79 - L80 were not covered by tests
cratedb_toolkit/docs/settings.py
[warning] 19-19: cratedb_toolkit/docs/settings.py#L19
Added line #L19 was not covered by tests
[warning] 26-27: cratedb_toolkit/docs/settings.py#L26-L27
Added lines #L26 - L27 were not covered by tests
[warning] 626-626: cratedb_toolkit/docs/settings.py#L626
Added line #L626 was not covered by tests
[warning] 631-631: cratedb_toolkit/docs/settings.py#L631
Added line #L631 was not covered by tests
cratedb_toolkit/docs/functions.py
[warning] 1-4: cratedb_toolkit/docs/functions.py#L1-L4
Added lines #L1 - L4 were not covered by tests
[warning] 6-12: cratedb_toolkit/docs/functions.py#L6-L12
Added lines #L6 - L12 were not covered by tests
[warning] 14-14: cratedb_toolkit/docs/functions.py#L14
Added line #L14 was not covered by tests
[warning] 16-16: cratedb_toolkit/docs/functions.py#L16
Added line #L16 was not covered by tests
[warning] 19-19: cratedb_toolkit/docs/functions.py#L19
Added line #L19 was not covered by tests
[warning] 22-27: cratedb_toolkit/docs/functions.py#L22-L27
Added lines #L22 - L27 were not covered by tests
[warning] 29-30: cratedb_toolkit/docs/functions.py#L29-L30
Added lines #L29 - L30 were not covered by tests
[warning] 32-32: cratedb_toolkit/docs/functions.py#L32
Added line #L32 was not covered by tests
[warning] 39-39: cratedb_toolkit/docs/functions.py#L39
Added line #L39 was not covered by tests
[warning] 42-45: cratedb_toolkit/docs/functions.py#L42-L45
Added lines #L42 - L45 were not covered by tests
[warning] 47-47: cratedb_toolkit/docs/functions.py#L47
Added line #L47 was not covered by tests
[warning] 57-59: cratedb_toolkit/docs/functions.py#L57-L59
Added lines #L57 - L59 were not covered by tests
[warning] 61-61: cratedb_toolkit/docs/functions.py#L61
Added line #L61 was not covered by tests
[warning] 68-68: cratedb_toolkit/docs/functions.py#L68
Added line #L68 was not covered by tests
[warning] 71-77: cratedb_toolkit/docs/functions.py#L71-L77
Added lines #L71 - L77 were not covered by tests
[warning] 80-81: cratedb_toolkit/docs/functions.py#L80-L81
Added lines #L80 - L81 were not covered by tests
[warning] 87-89: cratedb_toolkit/docs/functions.py#L87-L89
Added lines #L87 - L89 were not covered by tests
[warning] 91-91: cratedb_toolkit/docs/functions.py#L91
Added line #L91 was not covered by tests
[warning] 104-106: cratedb_toolkit/docs/functions.py#L104-L106
Added lines #L104 - L106 were not covered by tests
[warning] 108-109: cratedb_toolkit/docs/functions.py#L108-L109
Added lines #L108 - L109 were not covered by tests
[warning] 113-120: cratedb_toolkit/docs/functions.py#L113-L120
Added lines #L113 - L120 were not covered by tests
[warning] 126-126: cratedb_toolkit/docs/functions.py#L126
Added line #L126 was not covered by tests
[warning] 128-129: cratedb_toolkit/docs/functions.py#L128-L129
Added lines #L128 - L129 were not covered by tests
[warning] 131-132: cratedb_toolkit/docs/functions.py#L131-L132
Added lines #L131 - L132 were not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: build-and-publish
- GitHub Check: Kinesis: Python 3.12 on OS ubuntu-latest
- GitHub Check: Kinesis: Python 3.9 on OS ubuntu-latest
- GitHub Check: Generic: Python 3.12 on OS ubuntu-latest
- GitHub Check: Generic: Python 3.9 on OS ubuntu-latest
- GitHub Check: Generic: Python 3.8 on OS ubuntu-latest
- GitHub Check: CFR for OS windows-latest
- GitHub Check: CFR for OS ubuntu-latest
- GitHub Check: CFR for OS macos-13
🔇 Additional comments (12)
cratedb_toolkit/docs/functions.py (4)
22-31: LGTM: Well-structured Function dataclass.The Function class is well-designed with appropriate fields for representing CrateDB functions. The TODO comment accurately documents future parsing enhancements.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 22-27: cratedb_toolkit/docs/functions.py#L22-L27
Added lines #L22 - L27 were not covered by tests
[warning] 29-30: cratedb_toolkit/docs/functions.py#L29-L30
Added lines #L29 - L30 were not covered by tests
42-60: LGTM: Well-implemented FunctionRegistry.The FunctionRegistry provides a clean interface for managing functions with proper duplicate checking.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 42-45: cratedb_toolkit/docs/functions.py#L42-L45
Added lines #L42 - L45 were not covered by tests
[warning] 47-47: cratedb_toolkit/docs/functions.py#L47
Added line #L47 was not covered by tests
[warning] 57-59: cratedb_toolkit/docs/functions.py#L57-L59
Added lines #L57 - L59 were not covered by tests
71-77: LGTM: Appropriate implementation of sphinx_ref_role.The implementation handles Sphinx reference role correctly, extracting and processing text from the documentation.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 71-77: cratedb_toolkit/docs/functions.py#L71-L77
Added lines #L71 - L77 were not covered by tests
128-132: LGTM: Proper handling of empty registry.The check for an empty registry is correctly implemented, verifying if there are functions in the registry.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 128-129: cratedb_toolkit/docs/functions.py#L128-L129
Added lines #L128 - L129 were not covered by tests
[warning] 131-132: cratedb_toolkit/docs/functions.py#L131-L132
Added lines #L131 - L132 were not covered by testscratedb_toolkit/docs/cli.py (1)
22-38: LGTM: Well-documented help_functions.The help_functions documentation is clear and follows the same pattern as the existing help_settings. The examples are helpful and illustrate the command usage well.
cratedb_toolkit/util/format.py (2)
11-18: LGTM: Well-defined OutputFormat enum.The enum clearly defines the supported output formats and follows Python best practices.
24-64: LGTM: Clear and comprehensive format method.The format method handles both string and enum inputs well, with proper error messages and consistent behavior.
cratedb_toolkit/docs/settings.py (5)
19-19: No issues with the new import.This import statement is straightforward. Feel free to ignore any coverage warning related solely to this line, as it has no direct impact on the logic.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 19-19: cratedb_toolkit/docs/settings.py#L19
Added line #L19 was not covered by tests
26-27: No issues identified with the new imports.These imports correctly reference the new classes without introducing any logical or syntax errors.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 26-27: cratedb_toolkit/docs/settings.py#L26-L27
Added lines #L26 - L27 were not covered by tests
651-660: LGTM on the data fields.The introduction of
thingandformatteraligns well with theGenericProcessorpattern. The usage is clear and does not appear to introduce any issues.
664-677: Docstring indicates a program exit that doesn't occur.The docstring states "logs an error and exits the program" when no settings are found. However, the code only logs the error and continues. Confirm if the docstring or the behavior needs adjustment.
Do you want me to open a new issue or provide a potential fix to either raise an exception/sys.exit or update the docstring to reflect the current behavior?
684-701: The render logic looks solid.The updated docstring and runtime configurability logging are coherent. This method should be well covered through CLI tests for different output formats.
amotl
force-pushed
the
docs-api-functions
branch
from
f8f4803 to
902c427
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
cratedb_toolkit/docs/functions.py (1)
91-107: 🛠️ Refactor suggestionAdd error handling for network failures.
The
acquiremethod makes a network request without proper error handling for connection issues or HTTP errors.- document, pub = internals(requests.get(DOCS_URL, timeout=10).text) + try: + response = requests.get(DOCS_URL, timeout=10) + response.raise_for_status() # Raise exception for 4XX/5XX responses + document, pub = internals(response.text) + except (requests.RequestException, ConnectionError) as e: + logger.error(f"Failed to fetch documentation: {e}") + return self🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 91-91: cratedb_toolkit/docs/functions.py#L91
Added line #L91 was not covered by tests
[warning] 104-106: cratedb_toolkit/docs/functions.py#L104-L106
Added lines #L104 - L106 were not covered by tests
🧹 Nitpick comments (3)
cratedb_toolkit/docs/functions.py (1)
19-20: Hard-coded URL might need versioning flexibility.The URL points to a specific branch (
5.10), which could lead to maintenance issues when supporting multiple CrateDB versions.Consider making the URL configurable or version-aware:
-DOCS_URL = "https://github.com/crate/crate/raw/refs/heads/5.10/docs/general/builtins/scalar-functions.rst" +DOCS_URL_TEMPLATE = "https://github.com/crate/crate/raw/refs/heads/{version}/docs/general/builtins/scalar-functions.rst" +DOCS_URL = DOCS_URL_TEMPLATE.format(version="5.10") # Default version🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 19-19: cratedb_toolkit/docs/functions.py#L19
Added line #L19 was not covered by teststests/util/test_format.py (2)
32-37: Consider more thorough Markdown testingThe current test only checks for the presence of certain strings in the output. Consider testing the full structure of the Markdown output, especially for more complex nested dictionaries.
def test_formatter_markdown(): formatter = FlexibleFormatter(source_data) result = formatter.format("markdown") assert "## thing" in result assert "42.42" in result + # Verify the exact structure for more comprehensive testing + expected = "## thing\n\n42.42\n\n" + assert result == expected
1-56: Consider adding tests for additional scenariosThe test suite is comprehensive for basic functionality, but consider adding tests for:
- Nested dictionaries in Markdown format
- Unicode and special characters handling
- The
to_dictmethod to ensure deep copying works correctly- Larger or more complex data structures
def test_formatter_nested_dict_markdown(): nested_data = {"section": {"key1": "value1", "key2": 42}} formatter = FlexibleFormatter(nested_data) result = formatter.format("markdown") assert "## section" in result assert "### key1" in result assert "value1" in result assert "### key2" in result assert "42" in result def test_formatter_special_chars(): special_data = {"special": "üñíçødé & special chars: /\\\"'"} formatter = FlexibleFormatter(special_data) # Test that formatting doesn't corrupt special characters assert "üñíçødé" in formatter.format("json") assert "üñíçødé" in formatter.format("yaml") assert "üñíçødé" in formatter.format("markdown") def test_formatter_to_dict(): original = {"data": [1, 2, 3]} formatter = FlexibleFormatter(original) dict_copy = formatter.to_dict() # Verify it's a deep copy assert dict_copy is not original assert dict_copy["data"] is not original["data"] # Modify original and verify the copy is unchanged original["data"].append(4) assert len(dict_copy["data"]) == 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
CHANGES.md(1 hunks)cratedb_toolkit/docs/cli.py(3 hunks)cratedb_toolkit/docs/functions.py(1 hunks)cratedb_toolkit/docs/settings.py(3 hunks)cratedb_toolkit/docs/util.py(1 hunks)cratedb_toolkit/util/format.py(1 hunks)doc/docs-api.md(1 hunks)tests/docs/test_cli.py(1 hunks)tests/util/test_format.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- CHANGES.md
- doc/docs-api.md
- tests/docs/test_cli.py
- cratedb_toolkit/docs/util.py
🧰 Additional context used
🧠 Learnings (1)
cratedb_toolkit/docs/settings.py (2)
Learnt from: amotl
PR: crate/cratedb-toolkit#401
File: cratedb_toolkit/docs/settings.py:0-0
Timestamp: 2025-04-13T21:53:28.283Z
Learning: The project employs both direct unit tests (in files like tests/util/test_format.py testing the FlexibleFormatter class) and CLI integration tests for testing formatting components. Unit tests verify individual class methods while integration tests check end-to-end functionality.
Learnt from: amotl
PR: crate/cratedb-toolkit#401
File: cratedb_toolkit/docs/settings.py:0-0
Timestamp: 2025-04-13T21:53:28.283Z
Learning: The project employs both direct unit tests (in files like tests/util/test_format.py testing the FlexibleFormatter class) and CLI integration tests for testing formatting components. Unit tests verify individual class methods while integration tests check end-to-end functionality.
🧬 Code Graph Analysis (4)
cratedb_toolkit/docs/cli.py (2)
cratedb_toolkit/docs/functions.py (2)
FunctionsExtractor(81-132)acquire(91-132)cratedb_toolkit/docs/settings.py (3)
acquire(663-681)render(683-701)SettingsExtractor(651-701)
cratedb_toolkit/docs/functions.py (3)
cratedb_toolkit/docs/util.py (1)
GenericProcessor(9-61)cratedb_toolkit/model.py (1)
asdict(134-135)cratedb_toolkit/docs/cli.py (1)
functions(71-85)
tests/util/test_format.py (1)
cratedb_toolkit/util/format.py (3)
format(24-63)FlexibleFormatter(21-88)OutputFormat(11-17)
cratedb_toolkit/docs/settings.py (2)
cratedb_toolkit/docs/util.py (2)
GenericProcessor(9-61)render(19-36)cratedb_toolkit/util/format.py (5)
format(24-63)FlexibleFormatter(21-88)OutputFormat(11-17)to_markdown(74-85)to_sql(87-88)
🪛 GitHub Check: codecov/patch
cratedb_toolkit/docs/cli.py
[warning] 77-77: cratedb_toolkit/docs/cli.py#L77
Added line #L77 was not covered by tests
[warning] 79-85: cratedb_toolkit/docs/cli.py#L79-L85
Added lines #L79 - L85 were not covered by tests
[warning] 106-112: cratedb_toolkit/docs/cli.py#L106-L112
Added lines #L106 - L112 were not covered by tests
cratedb_toolkit/docs/functions.py
[warning] 1-4: cratedb_toolkit/docs/functions.py#L1-L4
Added lines #L1 - L4 were not covered by tests
[warning] 6-12: cratedb_toolkit/docs/functions.py#L6-L12
Added lines #L6 - L12 were not covered by tests
[warning] 14-14: cratedb_toolkit/docs/functions.py#L14
Added line #L14 was not covered by tests
[warning] 16-16: cratedb_toolkit/docs/functions.py#L16
Added line #L16 was not covered by tests
[warning] 19-19: cratedb_toolkit/docs/functions.py#L19
Added line #L19 was not covered by tests
[warning] 22-27: cratedb_toolkit/docs/functions.py#L22-L27
Added lines #L22 - L27 were not covered by tests
[warning] 29-30: cratedb_toolkit/docs/functions.py#L29-L30
Added lines #L29 - L30 were not covered by tests
[warning] 32-32: cratedb_toolkit/docs/functions.py#L32
Added line #L32 was not covered by tests
[warning] 39-39: cratedb_toolkit/docs/functions.py#L39
Added line #L39 was not covered by tests
[warning] 42-45: cratedb_toolkit/docs/functions.py#L42-L45
Added lines #L42 - L45 were not covered by tests
[warning] 47-47: cratedb_toolkit/docs/functions.py#L47
Added line #L47 was not covered by tests
[warning] 57-59: cratedb_toolkit/docs/functions.py#L57-L59
Added lines #L57 - L59 were not covered by tests
[warning] 61-61: cratedb_toolkit/docs/functions.py#L61
Added line #L61 was not covered by tests
[warning] 68-68: cratedb_toolkit/docs/functions.py#L68
Added line #L68 was not covered by tests
[warning] 71-77: cratedb_toolkit/docs/functions.py#L71-L77
Added lines #L71 - L77 were not covered by tests
[warning] 80-81: cratedb_toolkit/docs/functions.py#L80-L81
Added lines #L80 - L81 were not covered by tests
[warning] 87-89: cratedb_toolkit/docs/functions.py#L87-L89
Added lines #L87 - L89 were not covered by tests
[warning] 91-91: cratedb_toolkit/docs/functions.py#L91
Added line #L91 was not covered by tests
[warning] 104-106: cratedb_toolkit/docs/functions.py#L104-L106
Added lines #L104 - L106 were not covered by tests
[warning] 108-109: cratedb_toolkit/docs/functions.py#L108-L109
Added lines #L108 - L109 were not covered by tests
[warning] 113-120: cratedb_toolkit/docs/functions.py#L113-L120
Added lines #L113 - L120 were not covered by tests
[warning] 126-126: cratedb_toolkit/docs/functions.py#L126
Added line #L126 was not covered by tests
[warning] 128-129: cratedb_toolkit/docs/functions.py#L128-L129
Added lines #L128 - L129 were not covered by tests
[warning] 131-132: cratedb_toolkit/docs/functions.py#L131-L132
Added lines #L131 - L132 were not covered by tests
cratedb_toolkit/docs/settings.py
[warning] 19-19: cratedb_toolkit/docs/settings.py#L19
Added line #L19 was not covered by tests
[warning] 26-27: cratedb_toolkit/docs/settings.py#L26-L27
Added lines #L26 - L27 were not covered by tests
[warning] 626-626: cratedb_toolkit/docs/settings.py#L626
Added line #L626 was not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: build-and-publish
- GitHub Check: Generic: 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: CFR for OS ubuntu-latest
- GitHub Check: Generic: Python 3.8 on OS ubuntu-latest
- GitHub Check: CFR for OS macos-13
🔇 Additional comments (26)
cratedb_toolkit/docs/cli.py (4)
22-38: Well-structured function documentation for the new functions command.The
help_functionsdocumentation clearly explains the purpose and usage of the functions command with appropriate examples. The format follows the existing pattern in the codebase, making it consistent with other commands.
61-76: LGTM: Function command implementation follows CLI pattern.The new
functionscommand follows the established pattern in the file, with appropriate options for format and output, making it consistent with the existingsettingscommand.
79-85: Good error handling implementation.The try-except block with specific error messaging improves reliability and user experience by providing clear feedback when function extraction fails. This matches the error handling approach now also used in the settings command.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 79-85: cratedb_toolkit/docs/cli.py#L79-L85
Added lines #L79 - L85 were not covered by tests
106-112: Error handling consistency in settings command.Adding error handling to the settings command provides consistency with the functions command and improves the robustness of the CLI tool. The structured approach to error logging and propagation is good practice.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 106-112: cratedb_toolkit/docs/cli.py#L106-L112
Added lines #L106 - L112 were not covered by testscratedb_toolkit/util/format.py (4)
11-18: Good use of string-based Enum for output formats.Using a string-based enum for output formats provides type safety and self-documenting code. This makes it easy to validate input formats and extend with additional formats in the future.
20-64: Well-implemented format method with proper error handling.The
formatmethod effectively handles both string and enum format types, with clear error messages for unsupported formats. The error handling is thorough, catching ValueError exceptions and providing helpful context in the error message.
65-70: Defensive programming with deepcopy in to_dict.Using
deepcopyfor the returned data prevents unintended modifications to the original data structure, which is a good practice for methods that return mutable objects.
87-89: Clear NotImplementedError for to_sql method.Using NotImplementedError for the base
to_sqlmethod clearly indicates that domain-specific implementations should override this method, which is a good design pattern for abstract/base classes.cratedb_toolkit/docs/functions.py (3)
22-40: Well-structured Function dataclass with TODO for future enhancement.The
Functionclass is well-designed with essential fields and a clear TODO comment for future parsing of returns and example fields from the description. Theto_dictmethod is correctly implemented.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 22-27: cratedb_toolkit/docs/functions.py#L22-L27
Added lines #L22 - L27 were not covered by tests
[warning] 29-30: cratedb_toolkit/docs/functions.py#L29-L30
Added lines #L29 - L30 were not covered by tests
[warning] 32-32: cratedb_toolkit/docs/functions.py#L32
Added line #L32 was not covered by tests
[warning] 39-39: cratedb_toolkit/docs/functions.py#L39
Added line #L39 was not covered by tests
42-69: Good implementation of FunctionRegistry with duplicate detection.The registry implementation with validation against duplicates is a good practice. It will help prevent unexpected overwrites and maintain data integrity.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 42-45: cratedb_toolkit/docs/functions.py#L42-L45
Added lines #L42 - L45 were not covered by tests
[warning] 47-47: cratedb_toolkit/docs/functions.py#L47
Added line #L47 was not covered by tests
[warning] 57-59: cratedb_toolkit/docs/functions.py#L57-L59
Added lines #L57 - L59 were not covered by tests
[warning] 61-61: cratedb_toolkit/docs/functions.py#L61
Added line #L61 was not covered by tests
[warning] 68-68: cratedb_toolkit/docs/functions.py#L68
Added line #L68 was not covered by tests
108-132: Good use of CrateDB Toolkit metadata and function processing.The extraction logic is well-structured, with appropriate metadata tracking and clear function parsing from the document structure. The check for an empty registry before proceeding is also good practice.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 108-109: cratedb_toolkit/docs/functions.py#L108-L109
Added lines #L108 - L109 were not covered by tests
[warning] 113-120: cratedb_toolkit/docs/functions.py#L113-L120
Added lines #L113 - L120 were not covered by tests
[warning] 126-126: cratedb_toolkit/docs/functions.py#L126
Added line #L126 was not covered by tests
[warning] 128-129: cratedb_toolkit/docs/functions.py#L128-L129
Added lines #L128 - L129 were not covered by tests
[warning] 131-132: cratedb_toolkit/docs/functions.py#L131-L132
Added lines #L131 - L132 were not covered by testscratedb_toolkit/docs/settings.py (7)
19-19: Updated imports for new type annotations.The imports have been properly updated to include all required types for the class signatures. This improves type safety and code readability.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 19-19: cratedb_toolkit/docs/settings.py#L19
Added line #L19 was not covered by tests
26-27: Appropriate imports for supporting the new architecture.The imports of
GenericProcessorfromcratedb_toolkit.docs.utilandFlexibleFormatter,OutputFormatfromcratedb_toolkit.util.formatproperly support the inheritance and composition pattern used in the refactored code.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 26-27: cratedb_toolkit/docs/settings.py#L26-L27
Added lines #L26 - L27 were not covered by tests
591-605: Improved docstring for write_sql_statements function.The docstring has been significantly enhanced with more detailed information about purpose, parameters, and return values, which greatly improves code clarity and maintainability.
626-648: Well-designed OutputFormatter class extending FlexibleFormatter.The
OutputFormatterclass properly extendsFlexibleFormatterto provide domain-specific implementations for Markdown and SQL output formats. This follows good object-oriented design principles by extending the base class functionality.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 626-626: cratedb_toolkit/docs/settings.py#L626
Added line #L626 was not covered by tests
651-661: Class refactoring to use GenericProcessor inheritance.The
SettingsExtractorclass now properly inherits fromGenericProcessorand includes a formatter attribute defaulting toOutputFormatter. This refactoring aligns with the architectural changes to support both settings and functions extraction in a consistent way.
664-681: Improved acquire method with better structured docstring.The
acquiremethod has been updated to use the renamedthingattribute instead ofsettingsand includes a more detailed docstring. The error handling has been improved to log an error without terminating the program.
684-701: Simplified render method leveraging the base class.The
rendermethod has been simplified by delegating to the superclass implementation, which follows the DRY principle. The method still maintains the domain-specific logging of runtime configurable settings counts.tests/util/test_format.py (8)
1-7: Well-organized imports and good class importingThe imports are appropriately organized with standard libraries first, followed by third-party libraries, and then project-specific imports. The test directly imports the classes it needs to test.
8-10: Good test data setupThe test data is simple and focused, ideal for testing formatting functionality. Reusing the same test data across multiple test cases provides consistency.
12-16: Well-structured JSON format testThe test properly checks both string-based format specification and enum-based format specification, ensuring compatibility with both calling conventions.
18-23: Good error handling testThis test properly verifies that appropriate error messages are raised when invalid formats are provided, including checking the specific error message content.
25-30: Well-structured YAML format testSimilar to the JSON test, this properly verifies both string and enum format specifications for YAML output.
39-44: Good test for unimplemented formatsThis test correctly verifies that attempting to use the SQL format raises a NotImplementedError with the appropriate message.
46-49: Good edge case testing for empty dataTesting with empty data is important to ensure the formatter handles this edge case correctly.
51-56: Good test for type validation in Markdown formatterThis test correctly verifies that the Markdown formatter raises an appropriate error when given non-dictionary data.
amotl
force-pushed
the
docs-api-functions
branch
from
902c427 to
b93b65f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
cratedb_toolkit/docs/functions.py (1)
106-107: 🛠️ Refactor suggestionAdd error handling for network request.
The current implementation doesn't handle potential network failures or HTTP error responses when fetching documentation.
- document, pub = internals(requests.get(DOCS_URL, timeout=10).text) + try: + response = requests.get(DOCS_URL, timeout=10) + response.raise_for_status() # Raise exception for 4XX/5XX responses + document, pub = internals(response.text) + except (requests.RequestException, ConnectionError) as e: + logger.error(f"Failed to fetch documentation: {e}") + return self
🧹 Nitpick comments (5)
cratedb_toolkit/docs/settings.py (1)
663-681: Update docstring to match implementation.The docstring for
acquire()mentions that it exits the program if no settings are found, but the implementation just logs an error without exiting.- Extracts settings using extract_cratedb_settings() and assigns them to the - thing attribute. If no settings are found, logs an error and exits the program. - Otherwise, generates SQL statements for runtime configurable settings and - returns the SettingsExtractor instance. + Extracts settings using extract_cratedb_settings() and assigns them to the + thing attribute. If no settings are found, logs an error. Otherwise, generates + SQL statements for runtime configurable settings. Returns the SettingsExtractor + instance.tests/util/test_format.py (4)
32-37: Consider enhancing markdown validation.While checking for substrings works as a basic test, consider a more comprehensive approach to validate the structure of the Markdown output, ensuring headers are properly formatted and content is correctly indented.
def test_formatter_markdown(): formatter = FlexibleFormatter(source_data) result = formatter.format("markdown") - assert "## thing" in result - assert "42.42" in result + expected = "## thing\n\n42.42\n" + assert result.strip() == expected.strip()
58-67: Great test for nested dictionaries.This test ensures that nested dictionaries are properly formatted in Markdown, with appropriate header levels. Consider adding similar tests for JSON and YAML with nested structures.
def test_formatter_nested_dict_markdown(): nested_data = {"section": {"key1": "value1", "key2": 42}} formatter = FlexibleFormatter(nested_data) result = formatter.format("markdown") - assert "## section" in result - assert "### key1" in result - assert "value1" in result - assert "### key2" in result - assert "42" in result + # Verify headers and content structure exactly + lines = result.strip().split('\n') + assert lines[0] == "## section" + assert lines[2] == "### key1" + assert lines[4] == "value1" + assert lines[6] == "### key2" + assert lines[8] == "42"
69-76: Improve special character testing.Instead of checking for raw encoding, consider decoding the YAML output back to a dictionary and comparing it to the original data to ensure the roundtrip works correctly.
def test_formatter_special_chars(): special_data = {"special": "üñíçødé & special chars: /\\\"'"} formatter = FlexibleFormatter(special_data) # Test that formatting doesn't corrupt special characters - assert "üñíçødé" in formatter.format("json") - assert "\\xFC\\xF1\\xED\\xE7\\xF8d\\xE9" in formatter.format("yaml") - assert "üñíçødé" in formatter.format("markdown") + # Test JSON roundtrip + json_output = formatter.format("json") + assert json.loads(json_output) == special_data + + # Test YAML roundtrip + yaml_output = formatter.format("yaml") + assert yaml.safe_load(yaml_output) == special_data + + # For markdown, ensure the special characters are present + markdown_output = formatter.format("markdown") + assert "üñíçødé & special chars: /\\\"'" in markdown_output
1-88: Add test for handling None input.Consider adding a test case for handling
Noneas input to theFlexibleFormatterconstructor, as this is a common edge case that could occur in real usage.def test_formatter_with_none_input(): with pytest.raises(TypeError) as ex: FlexibleFormatter(None) # Adjust the expected error message to match the actual implementation assert "Cannot format None object" in str(ex.value)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
CHANGES.md(1 hunks)cratedb_toolkit/docs/cli.py(3 hunks)cratedb_toolkit/docs/functions.py(1 hunks)cratedb_toolkit/docs/settings.py(3 hunks)cratedb_toolkit/docs/util.py(1 hunks)cratedb_toolkit/util/format.py(1 hunks)doc/docs-api.md(1 hunks)tests/docs/test_cli.py(1 hunks)tests/util/test_format.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- CHANGES.md
- tests/docs/test_cli.py
- cratedb_toolkit/docs/util.py
- doc/docs-api.md
🧰 Additional context used
🧠 Learnings (1)
cratedb_toolkit/docs/settings.py (2)
Learnt from: amotl
PR: crate/cratedb-toolkit#401
File: cratedb_toolkit/docs/settings.py:0-0
Timestamp: 2025-04-13T21:53:28.283Z
Learning: The project employs both direct unit tests (in files like tests/util/test_format.py testing the FlexibleFormatter class) and CLI integration tests for testing formatting components. Unit tests verify individual class methods while integration tests check end-to-end functionality.
Learnt from: amotl
PR: crate/cratedb-toolkit#401
File: cratedb_toolkit/docs/settings.py:0-0
Timestamp: 2025-04-13T21:53:28.283Z
Learning: The project employs both direct unit tests (in files like tests/util/test_format.py testing the FlexibleFormatter class) and CLI integration tests for testing formatting components. Unit tests verify individual class methods while integration tests check end-to-end functionality.
🧬 Code Graph Analysis (4)
cratedb_toolkit/docs/cli.py (2)
cratedb_toolkit/docs/functions.py (2)
FunctionsExtractor(81-132)acquire(91-132)cratedb_toolkit/docs/settings.py (2)
acquire(663-681)SettingsExtractor(651-701)
cratedb_toolkit/docs/functions.py (3)
cratedb_toolkit/docs/util.py (1)
GenericProcessor(9-61)cratedb_toolkit/model.py (1)
asdict(134-135)cratedb_toolkit/docs/cli.py (1)
functions(71-85)
cratedb_toolkit/util/format.py (1)
cratedb_toolkit/docs/settings.py (2)
to_markdown(631-638)to_sql(640-647)
cratedb_toolkit/docs/settings.py (2)
cratedb_toolkit/docs/util.py (2)
GenericProcessor(9-61)render(19-36)cratedb_toolkit/util/format.py (5)
format(24-63)FlexibleFormatter(21-88)OutputFormat(11-17)to_markdown(74-85)to_sql(87-88)
🪛 GitHub Check: codecov/patch
cratedb_toolkit/docs/cli.py
[warning] 77-77: cratedb_toolkit/docs/cli.py#L77
Added line #L77 was not covered by tests
[warning] 79-85: cratedb_toolkit/docs/cli.py#L79-L85
Added lines #L79 - L85 were not covered by tests
[warning] 106-112: cratedb_toolkit/docs/cli.py#L106-L112
Added lines #L106 - L112 were not covered by tests
cratedb_toolkit/docs/functions.py
[warning] 1-4: cratedb_toolkit/docs/functions.py#L1-L4
Added lines #L1 - L4 were not covered by tests
[warning] 6-12: cratedb_toolkit/docs/functions.py#L6-L12
Added lines #L6 - L12 were not covered by tests
[warning] 14-14: cratedb_toolkit/docs/functions.py#L14
Added line #L14 was not covered by tests
[warning] 16-16: cratedb_toolkit/docs/functions.py#L16
Added line #L16 was not covered by tests
[warning] 19-19: cratedb_toolkit/docs/functions.py#L19
Added line #L19 was not covered by tests
[warning] 22-27: cratedb_toolkit/docs/functions.py#L22-L27
Added lines #L22 - L27 were not covered by tests
[warning] 29-30: cratedb_toolkit/docs/functions.py#L29-L30
Added lines #L29 - L30 were not covered by tests
[warning] 32-32: cratedb_toolkit/docs/functions.py#L32
Added line #L32 was not covered by tests
[warning] 39-39: cratedb_toolkit/docs/functions.py#L39
Added line #L39 was not covered by tests
[warning] 42-45: cratedb_toolkit/docs/functions.py#L42-L45
Added lines #L42 - L45 were not covered by tests
[warning] 47-47: cratedb_toolkit/docs/functions.py#L47
Added line #L47 was not covered by tests
[warning] 57-59: cratedb_toolkit/docs/functions.py#L57-L59
Added lines #L57 - L59 were not covered by tests
[warning] 61-61: cratedb_toolkit/docs/functions.py#L61
Added line #L61 was not covered by tests
[warning] 68-68: cratedb_toolkit/docs/functions.py#L68
Added line #L68 was not covered by tests
[warning] 71-77: cratedb_toolkit/docs/functions.py#L71-L77
Added lines #L71 - L77 were not covered by tests
[warning] 80-81: cratedb_toolkit/docs/functions.py#L80-L81
Added lines #L80 - L81 were not covered by tests
[warning] 87-89: cratedb_toolkit/docs/functions.py#L87-L89
Added lines #L87 - L89 were not covered by tests
[warning] 91-91: cratedb_toolkit/docs/functions.py#L91
Added line #L91 was not covered by tests
[warning] 104-106: cratedb_toolkit/docs/functions.py#L104-L106
Added lines #L104 - L106 were not covered by tests
[warning] 108-109: cratedb_toolkit/docs/functions.py#L108-L109
Added lines #L108 - L109 were not covered by tests
[warning] 113-120: cratedb_toolkit/docs/functions.py#L113-L120
Added lines #L113 - L120 were not covered by tests
[warning] 126-126: cratedb_toolkit/docs/functions.py#L126
Added line #L126 was not covered by tests
[warning] 128-129: cratedb_toolkit/docs/functions.py#L128-L129
Added lines #L128 - L129 were not covered by tests
[warning] 131-132: cratedb_toolkit/docs/functions.py#L131-L132
Added lines #L131 - L132 were not covered by tests
cratedb_toolkit/docs/settings.py
[warning] 19-19: cratedb_toolkit/docs/settings.py#L19
Added line #L19 was not covered by tests
[warning] 26-27: cratedb_toolkit/docs/settings.py#L26-L27
Added lines #L26 - L27 were not covered by tests
[warning] 626-626: cratedb_toolkit/docs/settings.py#L626
Added line #L626 was not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: build-and-publish
- GitHub Check: Generic: 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: Generic: Python 3.8 on OS ubuntu-latest
- GitHub Check: CFR for OS macos-13
🔇 Additional comments (23)
cratedb_toolkit/docs/cli.py (4)
22-38: Well-designed help documentation for the new functions command.The help documentation follows the established pattern, providing clear examples of the command usage with different output formats and file options. This maintains consistency across the command documentation.
40-57: Good update to the help_settings description.The description has been appropriately refined to specify "configuration settings" rather than just "settings", and the example output filename has been updated to match the more specific naming convention (
cratedb-settings.md).
61-85: Well-implemented functions command with proper error handling.The implementation follows the same pattern as the settings command, with appropriate options for format and output. The error handling is comprehensive, logging errors and raising click exceptions with informative messages.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 77-77: cratedb_toolkit/docs/cli.py#L77
Added line #L77 was not covered by tests
[warning] 79-85: cratedb_toolkit/docs/cli.py#L79-L85
Added lines #L79 - L85 were not covered by tests
106-112: Improved error handling for the settings command.Adding proper error handling to the settings command is a good improvement, making the behavior consistent with the new functions command and providing better user feedback when errors occur.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 106-112: cratedb_toolkit/docs/cli.py#L106-L112
Added lines #L106 - L112 were not covered by testscratedb_toolkit/docs/functions.py (3)
22-40: Well-designed Function dataclass.The
Functionclass has appropriate fields for representing CrateDB functions, with optional fields for future extensions. Theto_dict()method provides a clean way to convert instances to dictionaries.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 22-27: cratedb_toolkit/docs/functions.py#L22-L27
Added lines #L22 - L27 were not covered by tests
[warning] 29-30: cratedb_toolkit/docs/functions.py#L29-L30
Added lines #L29 - L30 were not covered by tests
[warning] 32-32: cratedb_toolkit/docs/functions.py#L32
Added line #L32 was not covered by tests
[warning] 39-39: cratedb_toolkit/docs/functions.py#L39
Added line #L39 was not covered by tests
42-68: Good implementation of FunctionRegistry.The registry provides proper validation when registering functions to prevent duplicates. The
to_dict()method enables easy serialization for output formatting.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 42-45: cratedb_toolkit/docs/functions.py#L42-L45
Added lines #L42 - L45 were not covered by tests
[warning] 47-47: cratedb_toolkit/docs/functions.py#L47
Added line #L47 was not covered by tests
[warning] 57-59: cratedb_toolkit/docs/functions.py#L57-L59
Added lines #L57 - L59 were not covered by tests
[warning] 61-61: cratedb_toolkit/docs/functions.py#L61
Added line #L61 was not covered by tests
[warning] 68-68: cratedb_toolkit/docs/functions.py#L68
Added line #L68 was not covered by tests
128-131: Appropriate handling of empty registry.The code correctly checks for empty registry by examining
self.registry.functionsinstead of justself.registry, which would always be truthy. The error handling logs the issue without terminating the program, allowing the CLI layer to handle the error appropriately.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 128-129: cratedb_toolkit/docs/functions.py#L128-L129
Added lines #L128 - L129 were not covered by testscratedb_toolkit/util/format.py (4)
11-18: Well-designed OutputFormat enum.Using a string-based enum for output formats provides type safety while allowing for string comparisons. This is a clean and effective approach.
24-63: Comprehensive format method with proper error handling.The
formatmethod handles both string and enum input, provides clear error messages for unsupported formats, and properly delegates to specific formatting methods.
74-85: Robust Markdown formatting.The Markdown formatter handles dictionaries appropriately, creating a structured output with sections and subsections. It also includes a type check to prevent errors when processing non-dictionary data.
87-88: Clear placeholder for SQL formatting.Using
NotImplementedErrorwith a descriptive message clearly indicates that domain-specific implementations need to provide their own SQL formatting logic.cratedb_toolkit/docs/settings.py (4)
26-27: Good refactoring with appropriate imports.The imports have been updated to use the new shared classes, supporting the refactoring to centralize common functionality.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 26-27: cratedb_toolkit/docs/settings.py#L26-L27
Added lines #L26 - L27 were not covered by tests
626-647: Well-designed OutputFormatter.The
OutputFormatterclass extendsFlexibleFormatterto provide domain-specific implementations for Markdown and SQL formatting, reusing existing functions. This is a clean approach that follows the extension pattern defined by the base class.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 626-626: cratedb_toolkit/docs/settings.py#L626
Added line #L626 was not covered by tests
651-661: Good refactoring of SettingsExtractor.The class now properly inherits from
GenericProcessor, with thesettingsfield renamed tothingto match the base class pattern. Theformatterfield defaults to the domain-specificOutputFormatter.
683-701: Simplified render method leveraging the base class.The
render()method now properly calls the superclass implementation and focuses only on the domain-specific logic of logging runtime configurable settings counts.tests/util/test_format.py (8)
1-10: Good setup and imports section.The imports are clean and well-organized. Having a simple test fixture (
source_data) is a good approach for reuse across multiple tests.
12-16: LGTM! Testing both string and enum inputs is thorough.Good test coverage for the JSON formatting functionality, validating both string-based and enum-based format specifications.
18-23: Excellent error case handling.Great job testing the error case with proper assertion on the error message. This ensures the formatter provides helpful guidance to users when they specify an invalid format.
39-44: Good test for not implemented format.Testing error cases for unimplemented formatters ensures helpful error messages for users.
46-49: LGTM! Good edge case testing.Testing with empty data is important for edge case coverage.
51-56: Thorough error handling for non-dictionary input.Good job testing the appropriate error case when non-dictionary data is provided for Markdown formatting.
78-88: Great deep copy validation.Excellent test to ensure that the
to_dictmethod performs a proper deep copy. This prevents unexpected side effects when modifying the original data.
25-30:Details
✅ Verification successful
Well-tested YAML formatting.
The test properly validates both string and enum format specifications. Just ensure that
sort_keys=Falsematches what's used in the actual implementation ofFlexibleFormatter.
🏁 Script executed:
#!/bin/bash # Check if the FlexibleFormatter implementation uses the same sort_keys parameter rg -A 3 -B 3 "yaml.dump" --glob "*.py" | grep -v "test_" | grep "sort_keys"Length of output: 586
Consistent YAML Formatting Implementation Detected
- The test in
tests/util/test_format.pycorrectly verifies YAML formatting for both the string specification ("yaml") and the enum (OutputFormat.YAML).- Verification shows that the implementation of
FlexibleFormatterincratedb_toolkit/util/format.pyusesyaml.dumpwithsort_keys=Falseconsistently.
amotl
force-pushed
the
docs-api-functions
branch
from
b93b65f to
06a4d88
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
cratedb_toolkit/docs/functions.py (1)
104-106: 🛠️ Refactor suggestionAdd error handling for network failures.
The HTTP request is made without any error handling, which could cause unexpected crashes if the URL is unreachable or returns an error status code.
Apply this diff to properly handle potential network issues:
- register_canonical_role("ref", sphinx_ref_role) - register_directive("seealso", Note) - document, pub = internals(requests.get(DOCS_URL, timeout=10).text) + register_canonical_role("ref", sphinx_ref_role) + register_directive("seealso", Note) + try: + response = requests.get(DOCS_URL, timeout=10) + response.raise_for_status() # Raise exception for 4XX/5XX responses + document, pub = internals(response.text) + except (requests.RequestException, ConnectionError) as e: + logger.error(f"Failed to fetch documentation: {e}") + return self🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 104-106: cratedb_toolkit/docs/functions.py#L104-L106
Added lines #L104 - L106 were not covered by tests
🧹 Nitpick comments (4)
cratedb_toolkit/docs/functions.py (2)
28-30: Consider implementing the TODO for extracting returns and examples.There's a TODO comment about parsing
returnsandexamplefields from the function description, which could enhance the extracted data's structure and usability.Would you like me to propose an implementation for parsing these fields from the function description? This could involve using regex patterns to identify return type descriptions and code examples in the documentation text.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 29-30: cratedb_toolkit/docs/functions.py#L29-L30
Added lines #L29 - L30 were not covered by tests
114-126: Consider adding type guard to improve type safety.Line 116 uses a type ignore comment which suggests a potential type issue. Consider adding a type guard to improve type safety.
for item in document: if item.tagname == "section": category_title = item.children[0].astext() - for function in item.children: # type: ignore[assignment] + for child in item.children: + if not isinstance(child, docutils.nodes.Element) or child.tagname != "section": + continue + function = child if function.tagname == "section": function_title = function.children[0].astext() function_body = function.children[1].astext()🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 126-126: cratedb_toolkit/docs/functions.py#L126
Added line #L126 was not covered by testscratedb_toolkit/util/format.py (1)
74-85: Improve the to_markdown method to handle edge cases better.The to_markdown method could be improved to handle additional edge cases, particularly when dealing with nested content structures.
Consider adding more checks and formatting options:
def to_markdown(self) -> str: if not isinstance(self.thing, dict): raise NotImplementedError(f"Unable to convert to Markdown: {self.thing}") buffer = io.StringIO() for section, content in self.thing.items(): buffer.write(f"## {section}\n\n") if isinstance(content, dict): for key, value in content.items(): + # Handle nested dictionaries with special formatting + if isinstance(value, dict) and any(isinstance(v, dict) for v in value.values()): + buffer.write(f"### {key}\n") + for subkey, subvalue in value.items(): + buffer.write(f"#### {subkey}\n```yaml\n{yaml.dump(subvalue, sort_keys=False)}```\n\n") + else: buffer.write(f"### {key}\n```yaml\n{yaml.dump(value, sort_keys=False)}```\n\n") else: - buffer.write(f"{str(content)}\n\n") + # Ensure proper formatting for non-dict content + buffer.write(f"{str(content)}\n\n") return buffer.getvalue()cratedb_toolkit/docs/settings.py (1)
663-681: Consider enhancing error handling in acquire method.Similar to the functions extractor, when no settings are extracted, an error is logged but the method still returns self, which could lead to silent failures.
Consider adding more robust error handling:
if self.thing: # Generate SQL statements for runtime configurable settings. generate_sql_statements(self.thing) else: logger.error("No settings were extracted. Please check the script or documentation structure.") + # Option 1: Raise an exception + # raise ValueError("No settings were extracted") + + # Option 2: Add a status field to track extraction success + # self.extraction_successful = False return self
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
CHANGES.md(1 hunks)cratedb_toolkit/docs/cli.py(3 hunks)cratedb_toolkit/docs/functions.py(1 hunks)cratedb_toolkit/docs/settings.py(3 hunks)cratedb_toolkit/docs/util.py(1 hunks)cratedb_toolkit/util/format.py(1 hunks)doc/docs-api.md(1 hunks)tests/docs/test_cli.py(1 hunks)tests/util/test_format.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- CHANGES.md
- doc/docs-api.md
- tests/docs/test_cli.py
- cratedb_toolkit/docs/util.py
- tests/util/test_format.py
🧰 Additional context used
🧠 Learnings (1)
cratedb_toolkit/docs/settings.py (2)
Learnt from: amotl
PR: crate/cratedb-toolkit#401
File: cratedb_toolkit/docs/settings.py:0-0
Timestamp: 2025-04-13T21:53:28.283Z
Learning: The project employs both direct unit tests (in files like tests/util/test_format.py testing the FlexibleFormatter class) and CLI integration tests for testing formatting components. Unit tests verify individual class methods while integration tests check end-to-end functionality.
Learnt from: amotl
PR: crate/cratedb-toolkit#401
File: cratedb_toolkit/docs/settings.py:0-0
Timestamp: 2025-04-13T21:53:28.283Z
Learning: The project employs both direct unit tests (in files like tests/util/test_format.py testing the FlexibleFormatter class) and CLI integration tests for testing formatting components. Unit tests verify individual class methods while integration tests check end-to-end functionality.
🧬 Code Graph Analysis (3)
cratedb_toolkit/docs/cli.py (2)
cratedb_toolkit/docs/functions.py (2)
FunctionsExtractor(81-132)acquire(91-132)cratedb_toolkit/docs/settings.py (3)
acquire(663-681)render(683-701)SettingsExtractor(651-701)
cratedb_toolkit/docs/functions.py (3)
cratedb_toolkit/docs/util.py (1)
GenericProcessor(9-61)cratedb_toolkit/model.py (1)
asdict(134-135)cratedb_toolkit/docs/cli.py (1)
functions(71-85)
cratedb_toolkit/docs/settings.py (2)
cratedb_toolkit/docs/util.py (2)
GenericProcessor(9-61)render(19-36)cratedb_toolkit/util/format.py (5)
format(24-63)FlexibleFormatter(21-88)OutputFormat(11-17)to_markdown(74-85)to_sql(87-88)
🪛 GitHub Check: codecov/patch
cratedb_toolkit/docs/cli.py
[warning] 77-77: cratedb_toolkit/docs/cli.py#L77
Added line #L77 was not covered by tests
[warning] 79-85: cratedb_toolkit/docs/cli.py#L79-L85
Added lines #L79 - L85 were not covered by tests
[warning] 106-112: cratedb_toolkit/docs/cli.py#L106-L112
Added lines #L106 - L112 were not covered by tests
cratedb_toolkit/docs/functions.py
[warning] 1-4: cratedb_toolkit/docs/functions.py#L1-L4
Added lines #L1 - L4 were not covered by tests
[warning] 6-12: cratedb_toolkit/docs/functions.py#L6-L12
Added lines #L6 - L12 were not covered by tests
[warning] 14-14: cratedb_toolkit/docs/functions.py#L14
Added line #L14 was not covered by tests
[warning] 16-16: cratedb_toolkit/docs/functions.py#L16
Added line #L16 was not covered by tests
[warning] 19-19: cratedb_toolkit/docs/functions.py#L19
Added line #L19 was not covered by tests
[warning] 22-27: cratedb_toolkit/docs/functions.py#L22-L27
Added lines #L22 - L27 were not covered by tests
[warning] 29-30: cratedb_toolkit/docs/functions.py#L29-L30
Added lines #L29 - L30 were not covered by tests
[warning] 32-32: cratedb_toolkit/docs/functions.py#L32
Added line #L32 was not covered by tests
[warning] 39-39: cratedb_toolkit/docs/functions.py#L39
Added line #L39 was not covered by tests
[warning] 42-45: cratedb_toolkit/docs/functions.py#L42-L45
Added lines #L42 - L45 were not covered by tests
[warning] 47-47: cratedb_toolkit/docs/functions.py#L47
Added line #L47 was not covered by tests
[warning] 57-59: cratedb_toolkit/docs/functions.py#L57-L59
Added lines #L57 - L59 were not covered by tests
[warning] 61-61: cratedb_toolkit/docs/functions.py#L61
Added line #L61 was not covered by tests
[warning] 68-68: cratedb_toolkit/docs/functions.py#L68
Added line #L68 was not covered by tests
[warning] 71-77: cratedb_toolkit/docs/functions.py#L71-L77
Added lines #L71 - L77 were not covered by tests
[warning] 80-81: cratedb_toolkit/docs/functions.py#L80-L81
Added lines #L80 - L81 were not covered by tests
[warning] 87-89: cratedb_toolkit/docs/functions.py#L87-L89
Added lines #L87 - L89 were not covered by tests
[warning] 91-91: cratedb_toolkit/docs/functions.py#L91
Added line #L91 was not covered by tests
[warning] 104-106: cratedb_toolkit/docs/functions.py#L104-L106
Added lines #L104 - L106 were not covered by tests
[warning] 108-109: cratedb_toolkit/docs/functions.py#L108-L109
Added lines #L108 - L109 were not covered by tests
[warning] 113-120: cratedb_toolkit/docs/functions.py#L113-L120
Added lines #L113 - L120 were not covered by tests
[warning] 126-126: cratedb_toolkit/docs/functions.py#L126
Added line #L126 was not covered by tests
[warning] 128-129: cratedb_toolkit/docs/functions.py#L128-L129
Added lines #L128 - L129 were not covered by tests
[warning] 131-132: cratedb_toolkit/docs/functions.py#L131-L132
Added lines #L131 - L132 were not covered by tests
cratedb_toolkit/docs/settings.py
[warning] 19-19: cratedb_toolkit/docs/settings.py#L19
Added line #L19 was not covered by tests
[warning] 26-27: cratedb_toolkit/docs/settings.py#L26-L27
Added lines #L26 - L27 were not covered by tests
[warning] 626-626: cratedb_toolkit/docs/settings.py#L626
Added line #L626 was not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: build-and-publish
- GitHub Check: Generic: Python 3.12 on OS ubuntu-latest
- GitHub Check: Generic: Python 3.9 on OS ubuntu-latest
- GitHub Check: Generic: Python 3.8 on OS ubuntu-latest
- GitHub Check: Kinesis: Python 3.12 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: CFR for OS macos-13
🔇 Additional comments (13)
cratedb_toolkit/docs/functions.py (2)
32-39: Code looks good, but consider usingasdictfrom dataclasses directly.The
to_dictimplementation is straightforward, but for consistency with Python's standard library patterns, you might consider usingasdictdirectly:def to_dict(self) -> Dict[str, Any]: """ Convert the dataclass instance to a dictionary. Returns: Dict[str, Any]: A dictionary containing all fields of the instance. """ - return dataclasses.asdict(self) + return dataclasses.asdict(self) # Alternative: from cratedb_toolkit.model import asdictThe current implementation works perfectly. This is just a note that there appears to be an
asdictmethod available in yourmodel.pymodule that you could use for consistency with other parts of your codebase.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 32-32: cratedb_toolkit/docs/functions.py#L32
Added line #L32 was not covered by tests
[warning] 39-39: cratedb_toolkit/docs/functions.py#L39
Added line #L39 was not covered by tests
71-77: The sphinx_ref_role implementation looks good.The custom Sphinx role implementation correctly handles the processing of references in the documentation, ensuring that the parser can process the CrateDB documentation format properly.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 71-77: cratedb_toolkit/docs/functions.py#L71-L77
Added lines #L71 - L77 were not covered by testscratedb_toolkit/docs/cli.py (4)
22-38: Documentation for functions command is clear and comprehensive.The help text for the functions command provides clear examples showing how to use different output formats and specify custom output files. This is consistent with the rest of the CLI documentation.
61-70: Command definition is well-structured.The command definition uses the
make_commanddecorator, proper option definitions, and consistent formatting with the rest of the codebase.
79-85: Good error handling implementation.The error handling adds proper exception handling, logging, and user-friendly error messages when extraction fails. This addresses the previous review comments about error handling.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 79-85: cratedb_toolkit/docs/cli.py#L79-L85
Added lines #L79 - L85 were not covered by tests
106-112: Consistent error handling applied to settings command.The same error handling pattern has been applied to the settings command, providing consistency across similar commands.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 106-112: cratedb_toolkit/docs/cli.py#L106-L112
Added lines #L106 - L112 were not covered by testscratedb_toolkit/util/format.py (3)
11-17: Well-defined OutputFormat enum.The OutputFormat enum clearly defines the supported output formats using an easy-to-understand enum that inherits from str, which makes string comparison operations clean and straightforward.
24-63: Robust format selection with good error handling.The format method properly handles format selection with appropriate error handling for unsupported formats. The error messages are clear and informative, including a list of supported formats.
87-88: NotImplementedError for to_sql is appropriate.Using NotImplementedError for the to_sql method is appropriate as it signals that subclasses need to provide a domain-specific implementation, which is consistent with the design pattern.
cratedb_toolkit/docs/settings.py (4)
626-648: OutputFormatter is a clean implementation of the formatting strategy.The OutputFormatter class neatly extends FlexibleFormatter and provides domain-specific implementations for Markdown and SQL formats. This adheres to good object-oriented design principles by inheriting common functionality and overriding only what needs customization.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 626-626: cratedb_toolkit/docs/settings.py#L626
Added line #L626 was not covered by tests
650-661: Good adaptation of SettingsExtractor to use GenericProcessor.The SettingsExtractor has been properly updated to inherit from GenericProcessor, maintaining the same functionality while leveraging the common implementation.
683-701: Simplified render method looks good.The render method has been simplified to leverage the superclass implementation while still maintaining the custom logging of runtime configurable settings count. This is a good example of code reuse and extension.
590-624: Improved docstring for write_sql_statements function.The updated docstring for write_sql_statements is more detailed and precise, clearly explaining the function's purpose, parameters, and return value, which improves code maintainability.
| if self.registry.functions: | ||
| self.thing = self.registry.to_dict() | ||
| else: | ||
| logger.error("No functions were extracted. Please check the script or documentation structure.") | ||
| return self |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix the empty registry check.
The condition if self.registry.functions: checks if the functions dictionary is non-empty, which is correct. However, when no functions are found, an error is logged but the method still returns self, potentially leading to silent failures downstream.
Consider either:
- Raising an exception when no functions are found, or
- Adding a status field to track extraction success
if self.registry.functions:
self.thing = self.registry.to_dict()
else:
logger.error("No functions were extracted. Please check the script or documentation structure.")
+ # Option 1: Raise an exception
+ raise ValueError("No functions were extracted")
+
+ # Option 2: Add a status field
+ # self.extraction_successful = False
return self📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if self.registry.functions: | |
| self.thing = self.registry.to_dict() | |
| else: | |
| logger.error("No functions were extracted. Please check the script or documentation structure.") | |
| return self | |
| if self.registry.functions: | |
| self.thing = self.registry.to_dict() | |
| else: | |
| logger.error("No functions were extracted. Please check the script or documentation structure.") | |
| # Option 1: Raise an exception | |
| raise ValueError("No functions were extracted") | |
| # Option 2: Add a status field | |
| # self.extraction_successful = False | |
| return self |
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 128-129: cratedb_toolkit/docs/functions.py#L128-L129
Added lines #L128 - L129 were not covered by tests
[warning] 131-132: cratedb_toolkit/docs/functions.py#L131-L132
Added lines #L131 - L132 were not covered by tests
amotl
changed the title
1 participant
About
Following up on GH-398 and GH-399, this adds an extractor for all CrateDB functions.
Documentation
Preview: https://cratedb-toolkit--401.org.readthedocs.build/docs-api.html#cratedb-functions