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

Add possibility to listen to logging events #30

Merged
merged 1 commit into from
Aug 19, 2023

Conversation

spudwebb
Copy link

I created methods to start and stop listening for logging events as per https://github.com/zwave-js/zwave-js-server#start-listening-to-logging-events
I did not implement the optional filter parameter as I'm not sure of how it works, and I couldn't find any documentation about it.

@marcus-j-davies
Copy link
Member

marcus-j-davies commented Aug 17, 2023

Thanks @spudwebb for the contribution - its always welcome!

Initial review looks great!

I'll review in more detail soon, I am aiming to update the library to the latest Server API offerings, so I may retarget this PR to the 4.0.0 Branch once I have fully reviewed, that aims to bring it up to date.

I want to update the web socket client code also, I'm not too keen on it currently.

until then - Many many thanks 👍

@marcus-j-davies
Copy link
Member

marcus-j-davies commented Aug 18, 2023

HI @spudwebb

I have had time to look at this.
I do have a comment..

I wonder if we should move this outside of the Driver class somehow?

You see, Im trying to remain faithful in the exposed methods that are offered by the Native JS Module and its namespaces.
I wonder if we can do something along the lines of...

Driver.ImplementationExtras.StartListeningLogs() - as its the server its self offering this command, and not the Driver.

But then in saying that, I have properties and events, that are not native in the Driver, so I'm questioning my own choices here 😅

@raman325
start_listening_logs is a server function right?, and isn't a mirror of any Driver function?

@spudwebb
Copy link
Author

@marcus-j-davies, I think I understand what you mean, but in the Z-Wave JS Server project itself this command is included in the DriverCommand enum and its full name is "driver.start_listening_logs" that' s why I added it to the Driver class.

Anyway I don' t have a strong preference, so whatever you think is best will be fine with me.

@marcus-j-davies
Copy link
Member

marcus-j-davies commented Aug 18, 2023

No problem.

I'll wait to see what @raman325 responds with.
if this is a utility method offered by the server its self, I'm tempted to move them to an ImplementationExtras class.

Keeping the Driver class as native as possible with its JS counterpart. 👍

@raman325
Copy link

that's a good point, yes this is a utility method that the server adds which creates a custom transport to forward the logs over the websocket. Something to consider updating in the server too, we have server level commands so there's an existing pattern to follow

@marcus-j-davies
Copy link
Member

Cool,
For the .NET library, I'm going to partition these utilities away from the Driver class.

@spudwebb,
Thanks for the PR - I'll merge it into the 4.0.0 branch, and will take it from there 👍

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.

3 participants