Add typed printcolumn argument to derive macro#1872
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1872 +/- ##
=======================================
- Coverage 76.3% 76.2% -0.0%
=======================================
Files 88 89 +1
Lines 8408 8487 +79
=======================================
+ Hits 6411 6465 +54
- Misses 1997 2022 +25
🚀 New features to boost your workflow:
|
|
I have been summoned, and I shall answer :) I can take a look at the PR later and leave some comments if there is anything worth pointing out. |
Techassi
left a comment
There was a problem hiding this comment.
Nicely done! Nothing major, just a few small things.
I wanted to tackle this, but didn't get around to it. So it is nice to see that someone else picked this up on their own 👍
I can confirm this is non-breaking and still supports to old raw JSON syntax after doing the review. |
|
Thanks for the quick feedback, sadly I was not able to come back to this earlier. |
|
I've run through you recent changes - nicely done. Everything from my comments was addressed. Sadly I cannot resolve my own comments, but I left comments indicating that they are resolved. Feel free to mark them as resolved (whoever is able to do that). I think the PR should then be ready to go. |
|
@cchndl Almost there! It looks like the only remaining issue is the DCO (Developer Certificate of Origin) sign-off. Per the CONTRIBUTING.md, each commit needs to be signed off using the You can fix this by amending your commits with: git rebase HEAD~<number_of_commits> --signoff
git push --force-with-lease@Techassi Thanks for the thorough review! Once the DCO check passes, I'll run the workflows. |
f9edc4b to
e5eb7c4
Compare
|
I think I made a mistake somewhere |
Allows specifying custom printer columns in a structured way analogous to scale in kube-rs#1656. Supplying a raw json string is still supported. Signed-off-by: cchndl <[email protected]>
use the new syntax in tests, add a test to check that the raw json produces the same output Extended the doc comments as suggested by Techassi Signed-off-by: cchndl <[email protected]>
e5eb7c4 to
b65097e
Compare
|
Sorry, I have not mixed merges and rebases until now, didn't know about |
|
@cchndl run just fmt please! |
Signed-off-by: cchndl <[email protected]>
|
Thank you for your patience! I hope it will go smoother next time. |
|
Not at all! It's impressive that you tackled such a large task and saw it through to the end on your first contribution. Thank you for the great work! |
Motivation
The raw json for the additional print columns is not not very nice to read, especially compared to the nice definition for the
scalesub-resource.Solution
Introduce the same structure from change #1656 for the printcolumn argument. I basically just traced the steps of that change for this one.
Tests are still missing, I just wanted to know first whether I should include both empty/present printcolumns and both the raw-json way and the new one introduced here.