Add alignment functionality to tables#171
Conversation
rolf-yoast
changed the title
danielbachhuber
left a comment
danielbachhuber
left a comment
There was a problem hiding this comment.
Thanks, @rolf-yoast !
Could you add some tests for this PR?
Also, it would be nice to address your TODO comment.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for column alignment in CLI tables. The implementation allows users to specify left, right, or center alignment for table columns using alignment constants defined in a new Column interface.
Key changes:
- Introduced a new
Columninterface with alignment constants mapped to PHP'sSTR_PAD_*constants - Added alignment tracking and configuration throughout the table rendering pipeline
- Modified the
Asciirenderer to apply column-specific alignments when padding cell content
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| lib/cli/table/Column.php | Defines new interface with alignment constants (ALIGN_LEFT, ALIGN_RIGHT, ALIGN_CENTER) |
| lib/cli/table/Renderer.php | Adds $_alignments property and setAlignments() method to base renderer class |
| lib/cli/table/Ascii.php | Implements alignment logic by storing headers and applying column-specific alignment in padColumn() |
| lib/cli/Table.php | Adds alignment parameter to constructor and propagates alignments to renderer |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public function setAlignments(array $alignments) { | ||
| $validAlignments = array(Column::ALIGN_LEFT, Column::ALIGN_RIGHT, Column::ALIGN_CENTER); | ||
| foreach ($alignments as $column => $alignment) { | ||
| if (!in_array($alignment, $validAlignments, true)) { | ||
| throw new \InvalidArgumentException("Invalid alignment value '$alignment' for column '$column'."); | ||
| } | ||
| if (!in_array($column, $this->_headers, true)) { | ||
| throw new \InvalidArgumentException("Column '$column' does not exist in table headers."); | ||
| } | ||
| } | ||
| $this->_alignments = $alignments; | ||
| } |
There was a problem hiding this comment.
The validation on line 222 checks if headers exist before alignments are set, but setAlignments() can be called in the constructor at line 68 before setHeaders() is called at line 64 when the constructor is invoked with an empty headers array and a non-empty alignments array. This will cause the validation to always fail in that case because $this->_headers will be empty. Consider either validating alignments lazily when they're used, or reordering the constructor calls, or skipping validation when headers are empty.
| if ($this->_headers === null) { | ||
| $this->_headers = array_values($row); | ||
| } |
There was a problem hiding this comment.
The header tracking logic assumes the first row rendered is always the headers, but this may not be reliable if the renderer is reused or if rows are rendered in an unexpected order. Consider passing headers explicitly to the renderer via a dedicated method (e.g., setHeaders()) rather than inferring them from the first row call to row().
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
- Create Column interface with ALIGN_LEFT, ALIGN_RIGHT, ALIGN_CENTER constants - Update Renderer base class to support alignments and headers - Update Ascii renderer to apply alignment when padding columns - Update Table class to support alignment in constructor and setAlignments() - Add comprehensive tests for all alignment types - Address PR #171 feedback: headers passed explicitly to renderer, validation allows setting alignments before headers Co-authored-by: swissspidy <841956+swissspidy@users.noreply.github.com>
|
Opened #192 to bring this over the finish line. Thanks for your contribution! |
Fixes #239