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

Extended the Interface #25

Closed
wants to merge 1 commit into from
Closed

Conversation

Sparksteam
Copy link
Contributor

Hi Oren - please review and consider these changes for incorporation into your master repository and package.

Added a Status flag in the resolver so clients can know if a scan is in
progress, and throw an exception if a Scan is initiated while a scan is
in progress. Added an indication of when scan is completed via the
callback. Test and handle cancelling scan more gracefully.

Thanks.

Added a Status flag in the resolver so clients can know if a scan is in
progress, and throw an exception if a Scan is initiated while a scan is
in progress. Added an indication of when scan is completed via the
callback. Test and handle cancelling scan more gracefully.
@Sparksteam Sparksteam mentioned this pull request Nov 4, 2015
@clairernovotny
Copy link
Collaborator

Can we change this to be done via an alternate scan method instead of changing the implementation?

My thoughts is that there is a more reliable way of conveying the same meaning, namely by using IObservable. I suspect that the existing method can be used and the callbacks translated into OnNext calls. When the task is done, OnCompleted would be called.

I'm pretty sure this can be done with a few lightweight classes instead of pulling in Rx, which I'd prefer not to do.

Thoughts?

@Sparksteam
Copy link
Contributor Author

Sorry Oren, what does “Rx” mean?

There’s certainly many ways to skin this cat. I tried to implement what I needed without further perturbation the of the ResolveAsync(), BrowseDomainsAsync() and callback method signatures, but I hear what you’re saying. Besides using the IObservable, event subscription pattern, here a couple of more options.

  1.   Expose a couple of events to which delegates can be assigned.
    
  2.   Add an EventType argument to the existing callback signature.
    

What do you think?

Thanks.

From: Oren Novotny [mailto:notifications@github.com]
Sent: Tuesday, November 10, 2015 5:11 AM
To: onovotny/Zeroconf Zeroconf@noreply.github.com
Cc: Sparksteam achin5957@hotmail.com
Subject: Re: [Zeroconf] Extended the Interface (#25)

Can we change this to be done via an alternate scan method instead of changing the implementation?

My thoughts is that there is a more reliable way of conveying the same meaning, namely by using IObservable. I suspect that the existing method can be used and the callbacks translated into OnNext calls. When the task is done, OnCompleted would be called.

I'm pretty sure this can be done with a few lightweight classes instead of pulling in Rx, which I'd prefer not to do.

Thoughts?


Reply to this email directly or view it on GitHub #25 (comment) . https://github.com/notifications/beacon/AL--vOaXJQsQL9HpRVypk_Sl3zC7k_p4ks5pEeRcgaJpZM4GbJHj.gif

@clairernovotny
Copy link
Collaborator

Can you please give version 2.4.1-observable01-bld013 a shot and let me know how it goes for you?

@Sparksteam
Copy link
Contributor Author

Seems pretty well behaved Oren, although I thought some anomalies when a scan is cancelled and restarted right away. It seemed like it was discovering a couple of less devices out of 44.

It could just be our network:(.

From: Oren Novotny [mailto:notifications@github.com]
Sent: Friday, November 13, 2015 1:34 PM
To: onovotny/Zeroconf Zeroconf@noreply.github.com
Cc: Sparksteam achin5957@hotmail.com
Subject: Re: [Zeroconf] Extended the Interface (#25)

Can you please give version 2.4.1-observable01-bld013 a shot and let me know how it goes for you?


Reply to this email directly or view it on GitHub #25 (comment) . https://github.com/notifications/beacon/AL--vE27p3yCWHOVrnLAp7CrT1bZ7-M3ks5pFk7dgaJpZM4GbJHj.gif

@clairernovotny
Copy link
Collaborator

Closing as this was done another way. Thanks!

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.

2 participants