fix: Darwin services ordering in DiscoverServices#417
fix: Darwin services ordering in DiscoverServices#417deadprogram merged 2 commits intotinygo-org:devfrom
Conversation
The services will now appear on the expected order
|
@deadprogram @jagobagascon review when you can please Thanks a lot 👍 @jagobagascon I've pinged you even though you were only mentioned on Windows reviews, but the change is basically the same |
|
@deadprogram can we merge this ? |
| svcs := []DeviceService{} | ||
|
|
||
| if len(uuids) > 0 { | ||
| // The caller wants to get a list of services in a specific | ||
| // order. | ||
| svcs = make([]DeviceService, len(uuids)) | ||
| } |
There was a problem hiding this comment.
This could be simplified:
| svcs := []DeviceService{} | |
| if len(uuids) > 0 { | |
| // The caller wants to get a list of services in a specific | |
| // order. | |
| svcs = make([]DeviceService, len(uuids)) | |
| } | |
| svcs := make([]DeviceService, len(uuids)) |
There was a problem hiding this comment.
I've used the same "algorithm" that was already implemented in other OS specific.
Should we change it in this PR ?
Should we change all algorithms ?
There was a problem hiding this comment.
I've used the same "algorithm" that was already implemented in other OS specific.
Should we change it in this PR ?
Should we change all algorithms ?
Perhaps we should look into seeing if we can use shared/improved code for all of them, but probably not in this PR.
| } else { | ||
| // The caller wants to get all services, in any order. | ||
| svcs = append(svcs, d.makeService(dsvcuuid, dsvc)) | ||
| } |
There was a problem hiding this comment.
maybe move this case to the beginning of the loop for clarity
if len(uuids) == 0 {
// The caller wants to get all services, in any order.
svcs = append(svcs, d.makeService(dsvcuuid, dsvc))
continue
}
| found = true | ||
| break | ||
| // One of the services we're looking for. | ||
| svcs[j] = d.makeService(dsvcuuid, dsvc) |
There was a problem hiding this comment.
the loop should probably break as soon as the service uuid is found, but this raises the question if duplicate uuids are allowed in the input of the method (which documentation does not prevent and the code does not check for).
There was a problem hiding this comment.
AFAIK duplicates should be allowed.
|
Thank your for working on this one @acouvreur @HattoriHanzo031 and @acouvreur perhaps working on a followup to standardize the ordering logic across all platforms would be a good thing! Now squash/merging. Thanks again! |
4 participants
The services will now appear on the expected order
Closes #416
Similar to #409 and its #410 pull request
Similar to #414 and its #415 pull request