Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a new list command to retrieve all DNS records from a domain zone, complementing the existing get command that retrieves a single record by ID.
Changes:
- Added
ListRecordsfunction to fetch and display all DNS records for a specified zone - Implemented cell width truncation (50 characters) with ellipsis for long table cell values
- Added comprehensive test coverage for the new list records functionality
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/services/domainzone/domainzone.go | Added ListRecords function and recordColumnsToDisplay variable to support listing DNS records |
| internal/display/display.go | Implemented max cell width truncation (50 chars) with ellipsis for both data tables and config tables |
| internal/cmd/domainzone_test.go | Added test case TestDomainZoneListRecords to verify the list records functionality |
| internal/cmd/domainzone.go | Registered new list subcommand under domain-zone record with filter flag support |
| doc/ovhcloud_domain-zone_record_list.md | Added documentation for the new list command with usage examples and flag descriptions |
| doc/ovhcloud_domain-zone_record.md | Updated parent command documentation to reference the new list subcommand |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
internal/display/display.go
Outdated
| } | ||
|
|
||
| // Extract values to display | ||
| const maxCellWidth = 50 |
There was a problem hiding this comment.
The magic number 50 for maxCellWidth is defined twice (lines 119 and 183). Consider defining it once at the package level to avoid duplication and ensure consistency.
internal/display/display.go
Outdated
|
|
||
| // Crop content if it exceeds max width | ||
| if len(cellValue) > maxCellWidth { | ||
| cellValue = cellValue[:maxCellWidth-3] + "…" |
There was a problem hiding this comment.
The magic number 3 for ellipsis length is hardcoded. Consider extracting it as a named constant (e.g., ellipsisLength = 3) to improve code clarity and maintainability.
baddfcd to
0957e3b
Compare
internal/display/display.go
Outdated
|
|
||
| const ( | ||
| maxCellWidth = 50 | ||
| ellipsisLength = 3 |
internal/display/display.go
Outdated
| } | ||
|
|
||
| // Crop content if it exceeds max width | ||
| if len(cellValue) > maxCellWidth { |
There was a problem hiding this comment.
a real truncate function should take into account UTF-8 runes to avoid truncating in the middle of a rune. I don't know if cellValue can contain such rune here.
There was a problem hiding this comment.
Something like:
func truncate(s string, maxLen int) string {
if len(s) <= maxLen {
return s
}
s = s[:maxLen-len("…")]
// Be sure we don't truncate in the middle of a UTF-8 rune
for {
r, size := utf8.DecodeLastRuneInString(s)
if r != utf8.RuneError {
return s + "…"
}
if size == 0 {
return "…"
}
s = s[:len(s)-size]
}
}There was a problem hiding this comment.
See also github.com/charmbracelet/x/ansi.Truncate as this module is already a dependency.
Signed-off-by: Marie JONES <14836007+marie-j@users.noreply.github.com>
0957e3b to
9bf417d
Compare
Description
Add ovhcloud domain-zone record list
Fixes #xx (issue)
Type of change
Please delete options that are not relevant.
Checklist:
make docgo mod tidy