refactor(core): extract format sub-package from internal/mycli#503
refactor(core): extract format sub-package from internal/mycli#503
Conversation
Break type coupling between formatters and core types by introducing FormatConfig struct, moving all formatter implementations to internal/mycli/format/ sub-package with clean interface boundary. Key changes: - New FormatConfig replaces *systemVariables in formatter signatures - StreamingFormatter interface takes []string column names instead of TableHeader - FinishFormat() simplified to take no arguments - Width calculation code extracted to format/width.go - Row type alias (format.Row = []string) for zero-breaking-change migration - format/ has zero imports from internal/mycli (no circular dependency) Files moved (6): formatters.go, formatters_common.go, formatters_table_streaming.go, formatters_html_xml_streaming.go, formatters_sql.go, formatters_sql_test.go New files (2): format/config.go, format/width.go Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Summary of ChangesHello @apstndb, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the core formatting logic by extracting it into a new, dedicated sub-package. This change aims to reduce tight coupling between formatters and core types, making the codebase more maintainable and testable. It centralizes formatting configurations and width calculation, streamlining how different output formats are handled throughout the application. Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Code Metrics Report📊 View detailed coverage report (available for 7 days)
Details | | main (09d51fa) | #503 (e257d5b) | +/- |
|---------------------|----------------|----------------|-------|
- | Coverage | 70.0% | 69.9% | -0.1% |
| Files | 54 | 55 | +1 |
| Lines | 5756 | 5712 | -44 |
- | Covered | 4031 | 3997 | -34 |
+ | Code to Test Ratio | 1:1.2 | 1:1.2 | +0.0 |
| Code | 13178 | 13131 | -47 |
+ | Test | 16492 | 16499 | +7 |
+ | Test Execution Time | 1m12s | 1m7s | -5s |Code coverage of files in pull request scope (75.6% → 75.6%)Reported by octocov |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request successfully refactors the formatting logic into a new internal/mycli/format package, improving architectural independence with FormatConfig. However, a critical security vulnerability exists in format.go where SQL comment display modes do not properly escape the */ sequence, potentially leading to SQL injection. Furthermore, the XML formatter's change to a streaming approach contradicts the repository's style guide, which mandates buffering for XML to ensure well-formed output.
Escape */ sequence in table content before wrapping in SQL comments to prevent premature comment closure and potential SQL injection. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
apstndb
force-pushed
the
extract-format-package
branch
from
1ef87a5 to
d725c90
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This is an excellent and large-scale refactoring that successfully extracts the formatting logic into a dedicated internal/mycli/format sub-package. The changes effectively decouple the formatters from core mycli types like *systemVariables and *Result by introducing the format.FormatConfig struct and new FormatFunc and StreamingFormatter interfaces.
The code is well-organized, and the new package structure is much cleaner. The introduction of the systemVariables.toFormatConfig() bridge method is a clean way to manage the transition. The simplification of the StreamingFormatter.FinishFormat interface by removing unused parameters is a great improvement.
I've reviewed the changes in detail, including the extraction of width calculation logic and the updates to all formatters and their callers. The changes are consistent and well-executed. The test suite has been updated accordingly, which gives confidence that the refactoring has not introduced regressions.
I could not find any issues of high or critical severity. This is a high-quality contribution. Great work!
|
/gemini summary |
Summary of ChangesThis pull request significantly refactors the application's core formatting logic by extracting it into a new, dedicated sub-package. This change aims to reduce tight coupling between various components, making the codebase more modular, maintainable, and testable. It centralizes formatting configurations and column width calculations, streamlining how different output formats are handled throughout the application while also addressing a critical security concern related to SQL comment handling. Highlights
Changelog
Activity
|
1 participant
Summary
Extract all formatter implementations from
internal/mycli/into a dedicatedinternal/mycli/format/sub-package, breaking the type coupling between formatters and core types (*systemVariables,*Result,TableHeader).This is Phase 3 of the
internal/mycli/restructuring (following #500 and #502).Key Changes
Row(type alias),FormatConfig,FormatFunc,StreamingFormatterinterfaceNewFormatter(),WriteTable(),WriteTableWithParams(),ExecuteWithFormatter(),NewStreamingFormatter(),TableParamsCSVFormatter,TabFormatter,VerticalFormatterwith updated signaturesTableStreamingFormatterusingFormatConfiginstead of*systemVariablesHTMLFormatter,XMLFormatterwith updated signaturesFormatSQL(),SQLStreamingFormatter, exported helper functionsCalculateWidth()and all width calculation helpers (~240 lines)toFormatConfig()bridge methodformat.FormatFuncandformat.StreamingFormatterDisplayModeUnspecifiedhandling for table formattingcreateStreamingProcessorForMode()thin wrapperDevelopment Insights
Discoveries
DisplayModeUnspecified(zero value) was not covered by the table-mode special case inprintTableData, causing column alignment to be lost for EXPLAIN/PROFILE output. Fixed by includingDisplayModeUnspecifiedin the table mode check.Rowtype needed to be a type alias (type Row = format.Row) rather than a defined type to maintain seamless compatibility across the package boundary.VerboseHeadersfield name (renamed fromVerboseNames) better conveys that these are full header strings that may contain newlines between column name and type name (e.g.,"Name\nSTRING").Test Plan
make checkpasses (test + lint + fmt-check)TestRenderPlanTree(column alignment preserved)format/package has zero imports frominternal/mycli(no circular dependency)go vet ./...cleanFixes #498