feat(table): mark columns as deprecated and show warning #1300
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1300 +/- ##
==========================================
- Coverage 72.50% 72.49% -0.01%
==========================================
Files 302 302
Lines 10995 11007 +12
==========================================
+ Hits 7972 7980 +8
- Misses 2150 2152 +2
- Partials 873 875 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
internal/cmd/base/list.go
Outdated
|
|
||
| warnings, err := t.ValidateColumns(cols) | ||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
This check was missing before. Since invalid columns were silently ignored before and now throw an error, this would be a breaking change. Do we want to keep it anyway?
There was a problem hiding this comment.
I usually prefer not to maintain bug compatibility.
But in this case, maybe we can log a warning?
There was a problem hiding this comment.
As far as I can tell the validation for all list commands is already implemented and works, so no breaking change. I think this is the code path currently:
base.ListCmd#CobraCommand():output.AddFlag(cmd, output.OptionNoHeader(), output.OptionColumns(outputColumns), output.OptionJSON(), output.OptionYAML())output.AddFlag():cmd.PreRunE = util.ChainRunE(cmd.PreRunE, validateOutputFlag(options))output.validateOutputFlag()checks if all specified columns are valid
Maybe this is something to clean up in a follow-up PR, to remove one of the validations and only have a single way to validate the table and --output flag.
There was a problem hiding this comment.
You're right, it already implements validation. validateOutputFlag() runs before t.ValidateColumns(cols), so we could just ignore the error and only output the warnings.
|
|
||
| // MarkFieldAsDeprecated marks the specified field as deprecated. The message will be printed | ||
| // to stderr if the column is used. | ||
| func (o *Table[T]) MarkFieldAsDeprecated(field string, message string) *Table[T] { |
There was a problem hiding this comment.
| func (o *Table[T]) MarkFieldAsDeprecated(field string, message string) *Table[T] { | |
| func (o *Table[T]) MarkColumnAsDeprecated(column string, message string) *Table[T] { |
I assume field means a cell in the context of the table, so we actually want to deprecate the column, and not the cell?
There was a problem hiding this comment.
It's consistent with the current naming. See this example:
// AddFieldFn adds a function which handles the output of the specified field.
func (o *Table[T]) AddFieldFn(field string, fn FieldFn[T]) *Table[T] {
o.fieldMapping[field] = fn
o.allowedFields[field] = true
o.columns[field] = true
return o
}There was a problem hiding this comment.
I think column is a better naming than field, but I'd like to keep it consistent to prevent confusion
There was a problem hiding this comment.
Lets go with field now, and feel free to open a PR to rename it consistently to Column :)
Closes #1295