-
-
Notifications
You must be signed in to change notification settings - Fork 403
Core search/list now return boards in a platform #268
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few nits but overall this is great, proto definition is much better now 👍
@@ -0,0 +1,37 @@ | |||
package core |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add license and copyright header?
commands/core/core.go
Outdated
|
||
// platformReleaseToRPC converts our internal structure to the RPC structure. | ||
// Note: this function does not touch the "Installed" field of rpc.Platform as it's not always clear that the | ||
// platformRelease we're currently converting is actually installed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: there's an extra tab at the beginning of this line
func platformReleaseToRPC(platformRelease *cores.PlatformRelease) *rpc.Platform { | ||
boards := make([]*rpc.Board, len(platformRelease.Boards)) | ||
i := 0 | ||
for _, b := range platformRelease.Boards { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's do comment not relevant since it's a mapfor i, b := range platformRelease.Boards
and let range
deal with the index increments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be my preferred way, too. Boards
is a map[string]..
though, thus i
would not be numeric index here unfortunately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops my bad, didn't check what's been iterated here, scratch my comment!
string ID = 1; | ||
string Version = 2; | ||
string Name = 3; | ||
repeated Platform search_output = 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🍰
Signed-off-by: Christian Weichel <[email protected]>
Signed-off-by: Christian Weichel <[email protected]>
This PR adds the list of boards supported by a platform, as well some more metadata to the response of
Core/List
andCore/Search
. Further, does it harmonise the structures used by both operations to de-duplicate conversion code converting from the internal structures to the RPC messages.