Skip to content
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

feat: add RSD instrumentation service support #396

Merged
merged 5 commits into from
Jun 10, 2024

Conversation

michal-przytarski
Copy link
Contributor

No description provided.

Copy link
Collaborator

@dmissmann dmissmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, they moved those things as they are to dtservicehub 🙂

@@ -82,6 +82,10 @@ func NewReadDevices() ReadDevicesType {
return data
}

func (device *DeviceEntry) SupportsRsd() bool {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exported methods should have some documentation

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done :D 22ba1b1

@@ -24,6 +25,10 @@ func (p loggingDispatcher) Dispatch(m dtx.Message) {
}

func connectInstruments(device ios.DeviceEntry) (*dtx.Connection, error) {
if device.SupportsRsd() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since you added a helper it would be nice to replace the checks for RSD in the other places as well

if device.Rsd == nil {

if device.Rsd == nil {

if device.Rsd == nil {

if device.Rsd == nil {

if device.Rsd == nil {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done :D 5dfc702

Copy link
Collaborator

@dmissmann dmissmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks! 🙇‍♂️

fish-sauce added a commit to kvs-coder/go-ios that referenced this pull request Apr 29, 2024
@michal-przytarski
Copy link
Contributor Author

Hi
What should we do with this PR? Pretty much all the changes are already merged. Only refactor left.
Do you want to merge it, or should I close it?

@dmissmann
Copy link
Collaborator

Do you want to merge it, or should I close it?

I would still merge it, but I can't. It says This branch is out-of-date with the base branch on my end and I assume that just means that you have to merge main into your branch, or rebase onto main

@danielpaulus danielpaulus merged commit 8bd7d9c into danielpaulus:main Jun 10, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants