WIP: Pass a list of NoteCategory to template instead of map of kind t… #1154
Conversation
|
Hi @j0n3lson. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/assign @saschagrunert |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: j0n3lson The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/ok-to-test |
| require.NoError(t, tt.collection.Sort(kindPriority)) | ||
| require.NoError(t, tmpl.Execute(&got, tt.collection), "rendering template") | ||
|
|
||
| require.Empty(t, diff.Diff(tt.wantOutput, got.String())) |
There was a problem hiding this comment.
What is the purpose of doing this instead of require.Equal() (should have a diff output, too)
|
|
||
| // Sort sorts selection by sorted by priority order. | ||
| func (n *NoteCollection) Sort(kindPriority []Kind) error { | ||
| // MAYBE? Implement sorted.Interface instead? |
There was a problem hiding this comment.
I assume you're referring the sort.Interface, right? What would be the benefit in implementing this interface?
There was a problem hiding this comment.
Yep. I left it as a place holder but I don't see a strong benefit to implementing sort.Interface to be honest. It was just a thought :)
There was a problem hiding this comment.
I'm happy with the (n *NoteCollection) Sort(kindPriority []Kind) method, it behaves in the same way like the other functions inside the sort package 👍
|
I think this appraoch goes into the right direction 👍, sorting maps in golang can be just done via slices, so I guess we're fine. |
|
/kind design |
There are two main changes here: * Bug fix: Template rendering was not rendering notes for PRs with no kinds (e.g uncategorized) * Design change: This eliminates the need to have an explicit NotesUncategorized field.
…o noteslice Introduces a NoteCollection type that can be passed to the template. This is need so that the kinds can be rendered in priority order (PR#1148). In the current implementation the template cannot render the notes in order because the NotesByKind map is unordered. If we like this idea I can fully implement it.
|
I added this to the other pr#1148 so I'm deleting this PR. |
changelog: remove changelog files from other releases when we cut the first official release
What type of PR is this?
What this PR does / why we need it:
Introduces a NoteCatetory that encapsulates the notes (string slice) and kind. Also introduces the NoteCollection type which is a container that is passed into the template. These are need to support #pr1148 as the template cannot sort the NotesByKind map easily.
If we like this idea I can fully implement it.
Which issue(s) this PR fixes:
None
Special notes for your reviewer:
Just wanted to demo the idea.
Does this PR introduce a user-facing change?: